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

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

On 9 May 2011 01:10, David Miller <davem@davemloft.net> wrote:
> From: Håkon Løvdal <hlovdal@gmail.com>
> Date: Mon, 9 May 2011 01:08:44 +0200
>
>> On 7 May 2011 21:35, matt mooney <mfmooney@gmail.com> wrote:
>>> But isn't the preferred style to have a single exit point?
>>
>> This is generally considered to be a bad advice, see
>> http://stackoverflow.com/questions/1701686/why-should-methods-have-a-single-entry-and-exit-points/1701721#1701721
>> for instance.
>
> That article totally ignores the issue of locking and how hard it is
> to get right without single exit points, and how unlocking in
> multiple spots bloats up the code.
>
> Definitely don't take that article's advice when working on the
> kernel.
>

I think we agree, but my answer was probably too short, unclear and
imprecise. In the case of locking and single exit points in the kernel,
they are (almost always) reached through goto/labels, and this is a fine
way of handling exiting a function, e.g.


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)
                goto out;

        //check if there are any slaves
        if (bond->slave_cnt == 0)
              goto re_arm;

        ...

re_arm:
        queue_delayed_work(bond->wq, &bond->ad_work, ad_delta_in_ticks);
out:
        read_unlock(&bond->lock);
}


I often advocate usage of goto to achive this kind of style.

What I assosiate with "writing a function as single exit style" would
be something like the following (and usually littered with
temporary remember-this-for-later variables).


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). So most probably my assosiasion was too
implicit to make my reply useful. That was not the intention, hopefully
this followup clears up a little.

BR Håkon Løvdal