netdev - Re: [PATCH] net/bonding: adjust codingstyle for bond

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 9 May 2011 02:08:41 +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 01:10, David Miller <davem@...emloft.net> wrote:
> From: Håkon Løvdal <hlovdal@...il.com>
> Date: Mon, 9 May 2011 01:08:44 +0200
>
>> On 7 May 2011 21:35, matt mooney <mfmooney@...il.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
--
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