| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 9 May 2011 03:30:10 +0200
From: Håkon Løvdal <hlovdal@...il.com>
To: David Miller <davem@...emloft.net>
Cc: mfmooney@...il.com, joe@...ches.com, aquini@...ux.com,
kernel-janitors@...r.kernel.org, fubar@...ibm.com,
andy@...yhouse.net, shemminger@...tta.com, netdev@...r.kernel.org,
nikai@...ai.net
Subject: Re: [PATCH] net/bonding: adjust codingstyle for bond_3ad files
On 9 May 2011 02:12, David Miller <davem@...emloft.net> wrote:
> From: Håkon Løvdal <hlovdal@...il.com>
> Date: Mon, 9 May 2011 02:08:41 +0200
>
>> void bond_3ad_state_machine_handler(struct work_struct *work)
>> {
>> struct bonding *bond = container_of(work, struct bonding,
>> ad_work.work);
>> struct port *port;
>> struct aggregator *aggregator;
>>
>> read_lock(&bond->lock);
>>
>> if (! bond->kill_timers) {
>>
>> //check if there are any slaves
>> if (bond->slave_cnt != 0) {
>> ...
>> }
>> queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
>> }
>> read_unlock(&bond->lock);
>> }
>>
>>
>> And this was what I trying to reccommend against (and which the
>> stackoverflow question is about).
>
> I really don't see anything wrong with either approach.
The example above looks innocent since it is so small. But written
this way, the all of core of the function ("...") will have two extra,
unnecessary indentation levels. My rule of thumb is to always test for
exceptions, not the normal case.
Expanding the example to cover the full function, it could be like the
following:
void bond_3ad_state_machine_handler(struct work_struct *work)
{
struct bonding *bond = container_of(work, struct bonding,
ad_work.work);
struct port *port;
struct aggregator *aggregator;
read_lock(&bond->lock);
if (! bond->kill_timers)
{
//check if there are any slaves
if (bond->slave_cnt != 0)
{
int uninitialized = 0;
// check if agg_select_timer timer after
initialize is timed out
if (BOND_AD_INFO(bond).agg_select_timer &&
!(--BOND_AD_INFO(bond).agg_select_timer)) {
// select the active aggregator for the bond
if ((port = __get_first_port(bond))) {
if (port->slave) {
aggregator =
__get_first_agg(port);
ad_agg_selection_logic(aggregator);
} else {
pr_warning("%s:
Warning: bond's first port is uninitialized\n",
bond->dev->name);
uninitialized = 1;
}
}
if (! uninitialized)|
bond_3ad_set_carrier(bond);
}
if (! uninitialized) {
// for each port run the state machines
for (port = __get_first_port(bond);
port; port = __get_next_port(port)) {
if (port->slave) {
/* Lock around state
machines to protect data accessed
* by all (e.g.,
port->sm_vars). ad_rx_machine may run
* concurrently due to
incoming LACPDU.
*/
__get_state_machine_lock(port);
ad_rx_machine(NULL, port);
ad_periodic_machine(port);
ad_port_selection_logic(port);
ad_mux_machine(port);
ad_tx_machine(port);
// turn off the BEGIN
bit, since we already handled it
if (port->sm_vars &
AD_PORT_BEGIN)
port->sm_vars
&= ~AD_PORT_BEGIN;
__release_state_machine_lock(port);
} else {
pr_warning("%s:
Warning: Found an uninitialized port\n",
bond->dev->name);
}
}
}
}
queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
}
read_unlock(&bond->lock);
}
Notice for instance "aggregator = __get_first_agg(port);" now have 5
levels of indentation in addition to the function body level, compared
to just 2 in the original.
That extra indentation is one thing, but the most important in my opinion
is that layout of the code becomes harder to read. This is because the
error handling code will typically be much more intrusive in the "layout".
Good code written like
if (error_check) {
error
handling
code
}
normal
code
here
is possible to quickly scan/read, but if code is written like
if (some_check) {
error
handling
code
} else {
normal
code
here
}
or
if (some_check) {
normal
code
here
} else {
error
handling
code
}
you have to go into slow gear and read and mentally process (relatively)
much more of the code to get the same picture. Combine a few such if/else,
nested - now everything is tightly intervened and impossible to quickly
grasp and/or hold in your head.
And the "uninitialized" variable is a crutch.
BR Håkon Løvdal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists