netdev - RE: [PATCH v3 1/4] ethernet: Add new driver for Marvell Armada 375 network unit

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:	Thu, 10 Jul 2014 15:26:55 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Ezequiel Garcia' <ezequiel.garcia@...e-electrons.com>,
	Francois Romieu <romieu@...zoreil.com>
CC:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	Jason Cooper <jason@...edaemon.net>,
	Marcin Wojtas <mw@...ihalf.com>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	Tawfik Bayouk <tawfik@...vell.com>,
	Lior Amsalem <alior@...vell.com>
Subject: RE: [PATCH v3 1/4] ethernet: Add new driver for Marvell Armada 375
 network unit

From: Ezequiel Garcia
...
> > > +/* Compare tcam data bytes with a pattern */
> > > +static bool mvpp2_prs_tcam_data_cmp(struct mvpp2_prs_entry *pe,
> > > +				    unsigned int offs, unsigned int size,
> > > +					   unsigned char *bytes)
> > > +{
> > > +	unsigned char byte, mask;
> > > +	int i;

Hmm. should be 'unsigned int' for the comparison against 'size'.

> > > +
> > > +	for (i = 0; i < size; i++) {
> > > +		mvpp2_prs_tcam_data_byte_get(pe, offs + i, &byte, &mask);
> > > +
> > > +		if (byte != bytes[i])
> > > +			return false;
> >
> > Please reduce the scope of "byte" and "mask".
> >
> 
> Agreed. This applies on several other places, I'll fix them all.
> 
> > > +	}
> > > +	return true;

Not sure about other people, but I prefer variables to be defined
at the top of a function so that I can find them.
Minimising the scope tends to make code harder to read.

If this was a big function, then reducing the scope of 'temporary'
variables need for a few lines (like the above loop) can make sense
because it reduces the clutter at the top of the function.

	David



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