linux-kernel - Re: [PATCH 12/15] kconfig: sort found symbols by relevance

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1373620065.4410.25.camel@chaos.site>
Date:	Fri, 12 Jul 2013 11:07:45 +0200
From:	Jean Delvare <jdelvare@...e.de>
To:	"Yann E. MORIN" <yann.morin.1998@...e.fr>
Cc:	Michal Marek <mmarek@...e.cz>, linux-kbuild@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Roland Eggner <edvx1@...temanalysen.net>,
	Wang YanQing <udknight@...il.com>
Subject: Re: [PATCH 12/15] kconfig: sort found symbols by relevance

Yann, Michal,

Le Wednesday 10 July 2013 à 22:46 +0200, Yann E. MORIN a écrit :
> Michal, All,
> 
> On 2013-07-08 19:35 +0200, Yann E. MORIN spake thusly:
> > On 2013-07-08 13:19 +0200, Jean Delvare spake thusly:
> > > Le Monday 24 June 2013 à 20:11 +0200, Yann E. MORIN a écrit :
> > > > From: "Yann E. MORIN" <yann.morin.1998@...e.fr>
> > [--SNIP--]
> > > > Since the search can be a regexp, it is possible that more than one symbol
> > > > matches exactly. In this case, we can't decide which to sort first, so we
> > > > fallback to alphabeticall sort.
> [--SNIP--]
> > > > Reported-by: Jean Delvare <jdelvare@...e.de>
> > > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@...e.fr>
> > > > Cc: Jean Delvare <jdelvare@...e.de>
> > > > Cc: Michal Marek <mmarek@...e.cz>
> > > > Cc: Roland Eggner <edvx1@...temanalysen.net>
> > > > Cc: Wang YanQing <udknight@...il.com>
> > > 
> > > I tested it and it works fine, thanks!
> > > 
> > > Tested-by: Jean Delvare <jdelvare@...e.de>
> > > 
> > > Now comes my late review. Overall I like the idea and the code but a few
> > > things could be improved:
> > 
> > Since this is already in Michal's tree, on should I proceed?
> >   - send an updated patch that replaces that one, or
> >   - send another additional patch with your proposed changes?
> 
> OK, since Michal already sent his pull-request to Linus, I'll prepare a
> corrective patch I'll submit before the end of the week. Is that OK with
> you, Michal?

Sounds good to me at least. I'll do my best to test and review it
quickly this time.

> [--SNIP--]
> > > > +static int sym_rel_comp( const void *sym1, const void *sym2 )
> > > > +{
> > > > +	struct sym_match *s1 = *(struct sym_match **)sym1;
> > > > +	struct sym_match *s2 = *(struct sym_match **)sym2;
> > > 
> > > You shouldn't need these casts.
> > 
> > Probably not, indeed, but I like to write (and read) what I expect to
> > happen, and pointer arithmetics is always something I dread to foobar.
> 
> In fact, we need the cast, otherwise gcc whines about const/non-const.

And quite rightly so, as you are taking const pointers (i.e. the caller
told you you are _not_ allowed to change the contents) and making them
non-const pointers (i.e. you give yourself the right to still change the
contents.) It happens that your function doesn't actually change the
contents, so no harm done, but this is still conceptually wrong.
Preserving the const nature of pointers down the call chain lets the
compiler warn you if a function changes data it was not supposed to.

So what you want to do is:

static int sym_rel_comp(const void *sym1, const void *sym2)
{
	const struct sym_match *s1 = sym1;
	const struct sym_match *s2 = sym2; 

This is both concise and correct, and it makes gcc happy.

Thanks,
-- 
Jean Delvare
Suse L3

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