Re: [PATCH] net/bonding: adjust codingstyle for bond_3ad files

["Håkon Løvdal" <hlovdal@gmail.com>]

On 9 May 2011 02:12, David Miller <davem@davemloft.net> wrote:
> From: Håkon Løvdal <hlovdal@gmail.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