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

Powered by blists - more mailing lists