netdev - Re: [PATCHv3 5/5] [NET] dsa: add support for the Marvell 88E6060 switch chip

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: <2acbd3e40810081320oe022835r7e6f53abc1147542@mail.gmail.com>
Date:	Wed, 8 Oct 2008 15:20:18 -0500
From:	"Andy Fleming" <afleming@...il.com>
To:	"Lennert Buytenhek" <buytenh@...tstofly.org>
Cc:	"Trent Piepho" <tpiepho@...escale.com>, netdev@...r.kernel.org
Subject: Re: [PATCHv3 5/5] [NET] dsa: add support for the Marvell 88E6060 switch chip

On Wed, Oct 8, 2008 at 3:49 AM, Lennert Buytenhek
<buytenh@...tstofly.org> wrote:
> On Tue, Oct 07, 2008 at 05:45:41PM -0700, Trent Piepho wrote:
>

>> > +#define REG_READ(addr, reg)                                        \
>> > +   ({                                                      \
>> > +           int __ret;                                      \
>> > +                                                           \
>> > +           __ret = reg_read(ds, addr, reg);                \
>> > +           if (__ret < 0)                                  \
>> > +                   return __ret;                           \
>> > +           __ret;                                          \
>> > +   })
>>
>> Macro with a hidden use of a local ('ds') and a hidden return?  The
>> former is discouraged (but can sure save a lot of typing) but the
>> latter seems like it's just begging to cause a missing unlock or
>> free on an error path.
>
> Yeah, well, that was intentional (Nicolas Pitre suggested it, and I
> figured it made sense to do it).  I think there are a couple more places
> in the tree that do this, and it very much increases readability because
> you no longer need to check the return value explicitly every single
> time you do an MII access.

I agree with Trent, it only increases readability in the sense that
you have to read less when scanning over the code.  To me, that is the
least important sort of readability.  This sort of macro just creates
hidden traps.  I know it's annoying to check that return value every
time, but that's why we get paid the big bucks.  ;)

If, for some reason, the community feels this is the right way to go,
at *least* rename the macro to make it clear that it returns on an
error, and make ds an explicit parameter.

Andy
--
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