lkml.org 
[lkml]   [2007]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2007-07-04 19:25    [W:4.079 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site