linux-kernel - Re: [PATCH] fixed a macro coding style issue

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: <20120725061512.GB6481@ZenIV.linux.org.uk>
Date:	Wed, 25 Jul 2012 07:15:12 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Baodong Chen <chenbdchenbd@...il.com>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fixed a macro coding style issue

On Tue, Jul 24, 2012 at 10:37:55PM -0700, Dmitry Torokhov wrote:
> On Wed, Jul 25, 2012 at 01:20:56PM +0800, Baodong Chen wrote:
> > Fixed a coding style issue in driver/input/input.c
> > 
> > Signed-off-by: Baodong Chen <chenbdchenbd@...il.com>
> > ---
> >  drivers/input/input.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index 8921c61..c96e983 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -845,11 +845,13 @@ int input_set_keycode(struct input_dev *dev,
> >  EXPORT_SYMBOL(input_set_keycode);
> >  
> >  #define MATCH_BIT(bit, max) \
> > +	do { \
> >  		for (i = 0; i < BITS_TO_LONGS(max); i++) \
> >  			if ((id->bit[i] & dev->bit[i]) != id->bit[i]) \
> >  				break; \
> > -		if (i != BITS_TO_LONGS(max)) \
> > -			continue;
> > +			if (i != BITS_TO_LONGS(max)) \
> > +				continue; \
> > +	} while (0)
> 
> This changes semantics. While current implementation of MATCH_BIT might
> not be great style, it is not supposed to be used outside of
> input_match_device(), and the replacement is completely broken.

Replacement is certainly completely broken, but "might not be great style"
is quite an understatement.  Everything else aside, it's too obfuscated
and ugly for anything outside of IOCCC and too verbose for IOCCC.

Damnit, at least turn that into
static inline int is_subset(unsigned long *bitmap1, unsigned long *bitmap2, size_t bits)
{
	int i;
	for (i = BITS_TO_LONGS(bits); i; bitmap1++, bitmap2++, i--)
		if ((*bitmap1 & *bitmap2) != *bitmap1)
			return 0;
	return 1;
}
and just before your loop
/* ignore ones that don't have bits required by id */
#define MATCH_BIT(array, bits) if (!is_subset(id->array, dev->array, bits)) continue;
with #undef MATCH_BIT right after the loop, to make it damn clean that it's local.
And frankly, I'd consider expanding that variant and killing the macro off.
Macros that affect control flow are vile and actively confusing for reader.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/