Re: [PATCH 18/25] dynamic_debug: Introduce global fake module param $module.dyndbg

[Jim Cromie]

On Tue, Dec 6, 2011 at 6:01 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> On Tue,  6 Dec 2011 12:11:28 -0700, jim.cromie@xxxxxxxxx wrote:
>> From: Jim Cromie <jim.cromie@xxxxxxxxx>
>>
>> Rework Thomas Renninger's $module.ddebug boot-time debugging feature,
>> from https://lkml.org/lkml/2010/9/15/397
>
> This worked out pretty neatly.  Thanks!
>

It did.  Almost like you planned it.

> A few questions from your patches:
>
>> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
>> index 7e3c53a..b77f43b 100644
>> --- a/include/linux/dynamic_debug.h
>> +++ b/include/linux/dynamic_debug.h
>> @@ -39,11 +39,16 @@ struct _ddebug {
>>  int ddebug_add_module(struct _ddebug *tab, unsigned int n,
>>                               const char *modname);
>>
>> +struct kernel_param;
>> +
>>  #if defined(CONFIG_DYNAMIC_DEBUG)
>>  extern int ddebug_remove_module(const char *mod_name);
>> +
>>  extern __printf(2, 3)
>>  int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
>
> struct kernel_param def belongs in future patch?  And whitespace change
> is kind of annoying.

kernel-param not needed - was part of old way.
whitespace cleaned out too.


>
>> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>> index 7939f63..91bd56a 100644
>> --- a/include/linux/moduleparam.h
>> +++ b/include/linux/moduleparam.h
>> @@ -292,7 +292,7 @@ extern int parse_args(const char *name,
>>                     char *args,
>>                     const struct kernel_param *params,
>>                     unsigned num,
>> -                   int (*unknown)(char *param, char *val));
>> +                   int (*unknown)(char *param, char *val, const char *modname));
>
> Do you really want the modname here, or a struct module?  I wonder if we
> should make this a standard opaque void *.  Of course, we'd have to
> modify parse_args callers, but you're abusing "name" arg here a bit
> anyway.
>

I thought about the possible abuse, but then saw existing uses:

linux-2.6]$ grep parse_args init/main.c
	parse_args("early options", cmdline, NULL, 0, do_early_param);
	parse_args("Booting kernel", static_command_line, __start___param,

"Booting kernel" isnt quite a name either.

wrt using a struct module arg, Id prefer not.
what I need is the name, so that the ddebug-queries being passed
can leave out the module name and thus be shorter and less redundant

forex:
foo.dyndbg=" func bar +p ; func buz +p ; func bang +p "

would otherwize need "module foo" in each of the 4 rules, which would
be tedious.
Since its been given already by user, and already available in load_module()'s
existing call to parse_args(), it seems straightforward to alter the
cb sig, and pass it down.



>> @@ -2893,7 +2893,8 @@ static struct module *load_module(void __user *umod,
>>         mutex_unlock(&module_mutex);
>>
>>         /* Module is ready to execute: parsing args may do that. */
>> -     err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
>> +     err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
>> +                      ddebug_dyndbg_param_cb);
>>         if (err < 0)
>>                 goto unlink;
>
> You have to
>

?? You left this thought unfinished.


>
>
>> @@ -510,8 +510,7 @@ asmlinkage void __init start_kernel(void)
>>       printk(KERN_NOTICE "Kernel command line: %s\n", boot_command_line);
>>       parse_early_param();
>>       parse_args("Booting kernel", static_command_line, __start___param,
>> -                __stop___param - __start___param,
>> -                &unknown_bootoption);
>> +                __stop___param - __start___param, &unknown_bootoption);
>>
>>       jump_label_init();
>
> Gratuitous whitespace change.
>

undone.

>> +int ddebug_dyndbg_param_cb(char *param, char* val, const char* modname)
>> +{
>> +     if (strcmp(param, "dyndbg")) {
>> +             pr_warn("bogus param %s=%s received for %s\n",
>> +                     param, val, modname);
>> +             return -EINVAL;
>> +     }
>
> This is already done in parse_args(); don't add a pr_warn here.
>

OK yes.  I'll change rc to -ENOENT.


> Thanks,
> Rusty.


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