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 netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
