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] [day] [month] [year] [list]
Date:	Fri, 22 Apr 2016 11:16:03 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	"Du, Changbin" <changbin.du@...el.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Jonathan Corbet <corbet@....net>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	"josh@...htriplett.org" <josh@...htriplett.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	"jiangshanlai@...il.com" <jiangshanlai@...il.com>,
	John Stultz <john.stultz@...aro.org>,
	Tejun Heo <tj@...nel.org>,
	"borntraeger@...ibm.com" <borntraeger@...ibm.com>,
	"dchinner@...hat.com" <dchinner@...hat.com>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/7] debugobjects: make fixup functions return bool
 instead of int

On Fri, 22 Apr 2016, Du, Changbin wrote:
> > On Fri, 22 Apr 2016, changbin.du@...el.com wrote:
> > > A bad thing is that debug_object_fixup use the return value for
> > > arithmetic operation. It confused me that what is the reall return
> > 
> > What's bad about that? The fact that it's used for arithmethic operation or
> > that it confused you?
> > 
> It confused me because this is not a common usage. I was confused that what
> does he fixup function return? A countable value? But doc says return fixed
> or not!

It says return 0 for not fixed up and 1 for fixed up. The activate fixup is
special and it has been written this way to handle the static initialization
case.

> 	if (fixup)
> 		fixed = fixup(addr, state);
> 	debug_objects_fixups += fixed;

> In common,for int return 0 indicates success, negative for fail, positive
> for something countable. So I think it is better follow this rule. Here is
> not of countable, it is Boolean.

Yes, it's common for most of the code. This code has been deliberately been
written differently. I'm not opposed to change that and improve it, but just
slapping bool on it does not really make any difference.

> So why not this?
> 	if (fixup && fixup(addr, state))
> 		debug_objects_fixups++;

There is no problem with that per se.
 
> > > +	bool (*fixup_init)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_activate)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_free)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_assert_init)(void *addr, enum debug_obj_state state);
> > 
> > So this change will introduce a gazillion of compile warnings because the
> > callbacks in the various usage sites are still having 'int' return type.
> > 
> No, I modified all the code who use debugojects API.

You do that in the later patches. But patches must be compilable and
functional on their own. Compiling this one will emit a gazillion of
"initialization from incompatible pointer type" warnings.

Thanks,

	tglx

Powered by blists - more mailing lists