Messages in this thread | | | Date | Wed, 4 Jul 2007 23:13:53 +0530 (IST) | From | Satyam Sharma <> | Subject | Re: [PATCH -mm 2/9] netconsole: Code simplification |
| |
On Wed, 4 Jul 2007, Matt Mackall wrote: > On Wed, Jul 04, 2007 at 04:37:49PM +0530, Satyam Sharma wrote: > > [...] > > (2) With this change, option_setup() is not required for modular netconsole. > > How is this a simplification? You've taken code with no #ifdefs and > added one! Please have both modular and nonmodular continue to use the > same paths.
No, modular and non-modular do *not* take the same paths even in the present code -- note that __setup() is a not even defined for modules.
If anything, the paths become somewhat "similar" *after* this patch.
Basically the present code is quite funny / strange, I'll explain why:
(1) For modular netconsole, the:
"module_param_string(netconsole, config, 256, 0);"
at the top of the file is what (effectively) does a:
strlcpy(config, "whatever", 256);
for us.
Note that __setup(option_setup) would be discarded at the preprocessor stage itself for a modular build.
Then, in init_netconsole(), we call netpoll_parse_options() through option_setup() ... the string (already copied into "config" due to the module_param_string as I mentioned above) is then parsed and the netpoll structure populated.
Then we proceed to do the netpoll_setup().
(2) For built-in netconsole:
__setup(option_setup) ensures a pointer to the option_setup() function is put into the .init.text section starting at __setup__start ... then, obsolete_checksetup() calls option_setup() with the appropriate string from the kernel's command line [note that all this is happening even before init_netconsole() gets called during the initcall process].
Anyway, obsolete_checksetup() calls option_setup() -> netpoll_parse_options() that parses the passed string options that matches after "netconsole=" [_not_ our "config" variable, which is a redundant/unused variable in the built-in case] and populates the "np" netpoll structure.
Then comes the kernel initcalls stage. init_netconsole() is duly called when its time comes, and sees strlen(config) == 0 (note that config is static and never populated in the built-in case that used the "netconsole=" boot option). So, options_setup() is *not* called. But that's no problem, "np" has already been populated after all.
And then we proceed to netpoll_setup() as usual.
Anyway, what I'm saying is that the _present_ code has radically different codepaths, and the proposed patch _is_ a simplification because it makes them somewhat more similar, as follows:
1. For modular netconsole, module_param_string effectively does a strcpy(config, "whatever", 256); for us.
2. For built-in case, __setup effectively does _the same_ for us.
And for *both* the cases, init_netconsole() will call the netpoll_parse_options() for us. Somewhat more similar, and simpler (at least for me -- taste varies, of course).
BTW, you *don't* really want the exact same codepaths for both built-in and modular netconsole anyway, believe me. If that's what you want, just get rid of the __setup use in netconsole -- module_param_string works equally good for modules when they are modular *or* when they are built-in.
HOWEVER, this will introduce a regression: people will have to specify the netconsole target specification as "netconsole.netconsole=..." on the kernel command line, and not simply as "netconsole=" as they have always been doing.
Perhaps I should've explained this a bit more verbosely in the changelog ...
Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |