Re: [tip:core/debugobjects] debugobjects: Be smarter about staticobjects

[Thomas Gleixner]

On Tue, 13 Dec 2011, Stephen Boyd wrote:
> 
> This code is only slightly confusing

Maybe we should tell that the guy who wrote it :)
 
> static int __init fixup_activate(void *addr, enum debug_obj_state state)
> {
>         struct self_test *obj = addr;
> 
>         switch (state) {
>         case ODEBUG_STATE_NOTAVAILABLE:
>                 if (obj->static_init == 1) {
>                         debug_object_init(obj, &descr_type_test);
>                         debug_object_activate(obj, &descr_type_test);
>                         /*
>                          * Real code should return 0 here ! This is
>                          * not a fixup of some bad behaviour. We
>                          * merily call the debug_init function to keep
>                          * track of the object.
>                          */
>                         return 1;
>                 } else {
>                         /* Real code needs to emit a warning here */
>                 }
>                 return 0;
> 
> 
> It seems that it does the complete opposite of what it should do, i.e.
> return 1 when the fixup is static and not actually a problem and return
> 0 otherwise. Because of this return 1, debug_object_activate() thinks
> there was a problem in the fixup and then it ups the warning count
> because this patch added a warning print for static objects.

Hmm, I think that was because I had not implemented that static
warning thing back then. So yes, it's backwards and should be fixed
proper:

> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index 77cb245..0ab9ae8 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -818,17 +818,9 @@ static int __init fixup_activate(void *addr, enum debug_obj_state state)   
>                 if (obj->static_init == 1) {
>                         debug_object_init(obj, &descr_type_test);
>                         debug_object_activate(obj, &descr_type_test);
> -                       /*
> -                        * Real code should return 0 here ! This is
> -                        * not a fixup of some bad behaviour. We
> -                        * merily call the debug_init function to keep
> -                        * track of the object.
> -                        */
> -                       return 1;
> -               } else {
> -                       /* Real code needs to emit a warning here */
> +                       return 0;
>                 }
> -               return 0;
> +               return 1;
> 
>         case ODEBUG_STATE_ACTIVE:
>                 debug_object_deactivate(obj, &descr_type_test);
> @@ -967,7 +959,7 @@ static void __init debug_objects_selftest(void)
> 
>         obj.static_init = 1;
>         debug_object_activate(&obj, &descr_type_test);
> -       if (check_results(&obj, ODEBUG_STATE_ACTIVE, ++fixups, warnings))
> +       if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
>                 goto out;
>         debug_object_init(&obj, &descr_type_test);
>         if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
> 
> 
> 
> This would make the fixup function for a static NOTAVAILABLE object
> return 0 and 1 appropriately and corrects the fixup and warning checking
> to reflect that nothing was in need of fixing.

Yes, the other thing works, but is butt ugly.
 
> Why was the fixup for selftest inverted?

See above plus laziness I assume :)

Can you please resend with a changelong ?

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/