RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Thomas Gleixner]

On Fri, 22 Apr 2016, Du, Changbin wrote:
> > On Fri, 22 Apr 2016, changbin...@intel.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

RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Thomas Gleixner]

On Fri, 22 Apr 2016, Du, Changbin wrote:
> > On Fri, 22 Apr 2016, changbin...@intel.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

RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Du, Changbin]

Hi,
> On Fri, 22 Apr 2016, changbin...@intel.com wrote:
> > From: "Du, Changbin" 
> >
> > 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

RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Du, Changbin]

Hi,
> On Fri, 22 Apr 2016, changbin...@intel.com wrote:
> > From: "Du, Changbin" 
> >
> > 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

Re: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Thomas Gleixner]

On Fri, 22 Apr 2016, changbin...@intel.com wrote:
> From: "Du, Changbin" 
> 
> 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?

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

Thanks,

tglx

Re: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Thomas Gleixner]

On Fri, 22 Apr 2016, changbin...@intel.com wrote:
> From: "Du, Changbin" 
> 
> 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?

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

Thanks,

tglx