From: "Du, Changbin" <changbin.du@intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
CC: Andrew Morton <akpm@linux-foundation.org>,
        Jonathan Corbet <corbet@lwn.net>,
        "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
        "josh@joshtriplett.org" <josh@joshtriplett.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "jiangshanlai@gmail.com" <jiangshanlai@gmail.com>,
        John Stultz <john.stultz@linaro.org>, Tejun Heo <tj@kernel.org>,
        "borntraeger@de.ibm.com" <borntraeger@de.ibm.com>,
        "dchinner@redhat.com" <dchinner@redhat.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 1/7] debugobjects: make fixup functions return bool
 instead of int
Thread-Topic: [PATCH 1/7] debugobjects: make fixup functions return bool
 instead of int
Thread-Index: AQHRnG+qzxCyMCbdb0yG3GlQ1KOogJ+VJC4AgACH9qA=
Date: Fri, 22 Apr 2016 08:54:24 +0000
Message-ID: <0C18FE92A7765D4EB9EE5D38D86A563A05D1B3D7@SHSMSX103.ccr.corp.intel.com>
References: <1461312468-14335-1-git-send-email-changbin.du@intel.com>
 <1461312468-14335-2-git-send-email-changbin.du@intel.com>
 <alpine.DEB.2.11.1604221032420.3941@nanos>
In-Reply-To: <alpine.DEB.2.11.1604221032420.3941@nanos>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2061
Lines: 53

Hi,
> On Fri, 22 Apr 2016, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > The object debugging infrastructure core provides some fixup callbacks
> > for the subsystem who use it. These callbacks are called from the debug
> > code whenever a problem in debug_object_init is detected. And
> > debugobjects core suppose them returns 1 when the fixup was successful,
> > otherwise 0. So the return type is boolean.
> >
> > 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!
	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.
So why not this?
	if (fixup && fixup(addr, state))
		debug_objects_fixups++;

> > Reading over the whole code, I found some place do use the return value
> > incorrectly(see next patch). So why use bool type instead?
> 
> Patches which fix a problem need to come first not in the middle of a revamp
> series.
> 
Thanks, I am first know of this.

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

> Thanks,
> 
> 	tglx

Thanks,
Du, Changbin