On 9 May 2011 02:12, David Miller <davem@xxxxxxxxxxxxx> wrote:
> From: HÃkon LÃvdal <hlovdal@xxxxxxxxx>
> 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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
