linux-kernel - Re: [PATCH 02/27] drivers/net: fix sparse warnings: make do-while a compound statement

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]

Message-ID: <a01a16b50812221544x45c3559ap808ac0095ee8f01@mail.gmail.com>
Date:	Tue, 23 Dec 2008 00:44:25 +0100
From:	"Håkon Løvdal" <hlovdal@...il.com>
To:	"Krzysztof Halasa" <khc@...waw.pl>
Cc:	"Hannes Eder" <hannes@...neseder.net>, netdev@...r.kernel.org,
	kernel-janitors@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/27] drivers/net: fix sparse warnings: make do-while a compound statement

2008/12/22 Krzysztof Halasa <khc@...waw.pl>:
>> --- a/drivers/net/starfire.c
>> +++ b/drivers/net/starfire.c
>> @@ -882,9 +882,9 @@ static int mdio_read(struct net_device *dev, int phy_id, int location)
>>       void __iomem *mdio_addr = np->base + MIICtrl + (phy_id<<7) + (location<<2);
>>       int result, boguscnt=1000;
>>       /* ??? Should we add a busy-wait here? */
>> -     do
>> +     do {
>>               result = readl(mdio_addr);
>> -     while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0);
>> +     } while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0);
>>       if (boguscnt == 0)
>>               return 0;
>>       if ((result & 0xffff) == 0xffff)
>
>
> I don't know how one could think the above is an improvement.

For this specific issue the important aspect is to improve readability
(and not just eventually satisfy some warning from a tool), which I
assume there is no disagrement on. Now what constitutes improved
readbility on the other hand is another issue, and I guess there are
next to as many oppinions as developers... :)
I would most certainly prefer to have code look consistently and always
have brackets, even in the case of just one body line.

Of secondary importance is the benefit that always using brackets
makes them much more merge friendly. Consider the following scenario:

Common base:
        do
                result = readl(mdio_addr);
        while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0);

Branch 1
        do {
                result = readl(mdio_addr);
                do_something();
        } while ((result & 0xC0000000) != 0x80000000 && --boguscnt > 0);

Branch 2
        do
                result = readl(mdio_addr);
        while (NOT_OK(result) && --boguscnt > 0);

In this case if both branch 1 and 2 are merged, there will be a merge conflict
that has to be resolved manually. If the brackets had been in place from the
start tools should be able to resolv this automatically.

BR Håkon Løvdal