Re: module-init-tools: [GIT v2] softdep command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 5, 2009 at 10:28 AM, Alan Jenkins
<sourcejedi.lkml@xxxxxxxxxxxxxx> wrote:
> On 9/25/09, Andreas Robinson <andr345@xxxxxxxxx> wrote:
>> Anyway, care for some light weekend reading?
>
> One other thing
>
>>    * Failures in the indirect modprobes, ie. those run by the softdep
>>      command, are ignored. This seems correct for removing, but I'm
>>      unsure what the proper action is when installing.
>
> I think it's the correct behaviour for install commands as well.

That sounds good, in that case I'm happy what's written so far. The
comments you had have been addressed (below), so it looks a lot closer
to being finished.

I'm working on the tests right now, but what's pushed to github ought
to be the final versions.

(Who, me, arrogant? No, of course the code is flawless now, before
it's even tested ... )

Joking aside, if you're happy with this round, any bugfixes will go
into additional commits so you don't have to review everything again.

On Thu, Oct 1, 2009 at 4:54 PM, Alan Jenkins
<sourcejedi.lkml@xxxxxxxxxxxxxx> wrote:
> I can't help noticing you removed the loop detection.  I suppose it's
> not needed now do_softdeps() is called in the same place as
> do_commands().

Maybe, but I've re-added the loop detector anyway, just in case. If it
can be broken, someone will...

>  BTW, the recent fixes for loopy install commands will
> conflict with your work.  It should be easy to resolve with git-merge,
> but I'm happy to help if you're not sure.

Thanks. I was able to resolve it with good old "patch" and looking at
the rejects though. git-merge and I are not friends...

> I think almost all of the fatal() calls are ok.  I think it's fine to
> abort for NOFAIL allocations, missing or corrupt indexes, and missing
> configuration files.  There are just two suspicious ones where
> load_strings() is called with fatal() as the error function.

[...greps snipped...]

Thanks for looking at this. It's what I fretted about most.

The fatal calls in load_strings() are resolved in the second-to-last
commit. Basically, I got rid of that parameter entirely. IIRC, it was
added as a simple way to detect file corruption. But we could add a
file checksum in modpost instead, perhaps?

The way load_strings() is written now, buffer overruns can't happen
anyway, so at least it's safe.
At worst, the kernel might complain about missing symbols and such.

> I think you should omit the alias- and symfilename and calculate them
> as needed.  You could change read_aliases_file() to take dirname, and
> a _relative_ filename which could then be a string literal
> ("modules.alias").

Good idea. Done.

> @@ -780,6 +791,37 @@ static int parse_config_file(const char *filename,
>                                *commands = add_command(underscores(modname),
>                                                        ptr, *commands);
[...]
> I think the lifecycle details could be a bit clearer if each field in
> "new" was explicitly assigned at the end.  Also, it appears to leak
> the string tables on syntax errors.

Done and fixed.

> Ew, casting away const.  We can avoid this by moving underscores() out
> of do_modprobe.    Please put this on your TODO list if it's not there
> already :-).

Too late for the TODO, already fixed it :) in this commit:
"modprobe: don't modify the modname string passed to do_modprobe"

do_modprobe now copies the modname and the recast isn't needed.

------------------------------

The pull request:

The following changes since commit fdae0df92cf48e02b89376a67e405191e2949977:
  Jon Masters (1):
        module-init-tools v3.11-rc1

are available in the git repository at:

  git://github.com/andr345/module-init-tools.git modprobe-softdep

Andreas Robinson (8):
      modprobe: reduce nesting in conf parser
      modprobe: cleanup indentation change in conf parser
      modprobe: put configuration objects in a struct
      modprobe: enable calling do_modprobe from within
                handle_module,insmod,rmmod
      modprobe: don't modify the modname string passed to do_modprobe
      modprobe: add softdep command
      elfops: remove errfn_t from load_strings
      modprobe: add simple softdep loop detector

 depmod.c      |    8 +-
 elfops.h      |    2 +-
 elfops_core.c |   11 +-
 modinfo.c     |    2 +-
 modprobe.c    |  450 +++++++++++++++++++++++++++++++++++++++------------------
 5 files changed, 321 insertions(+), 152 deletions(-)

---------------------------

And the log:

commit e13a554bb4b175f57253e613007a9c50d69bbbe8
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sat Oct 3 20:07:15 2009 +0200

    modprobe: reduce nesting in conf parser

    Helps readability and will simplify the softdep command parser.

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit bc334ddccd70b30f79983d1dff6eef2e652d054a
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sat Oct 3 20:18:35 2009 +0200

    modprobe: cleanup indentation change in conf parser

    Follow up to the previous commit.

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit 62208953d789c8510ecb8adfe0a564bcc1ebbdfd
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sat Oct 3 20:22:48 2009 +0200

    modprobe: put configuration objects in a struct

    It is now possible to add or remove conf commands without rewriting
    the function declarations of e.g parse_config_*() every time.

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit 13256ef2df49f613ad6b7de7617001ed464229ff
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sat Oct 3 20:39:21 2009 +0200

    modprobe: enable calling do_modprobe from within handle_module,insmod,rmmod

    A new function, do_softdep(), will be invoked from the same locations
    as do_command() as it eventually will replace it. do_softdep() in turn
    needs to call do_modprobe().

    This commit adds some parameters to do_modprobe, handle_module,
    insmod and rmmod, needed to make this possible.

    The pathnames for "module.symbols" and "modules.alias" are now
    generated wherever they're needed, rather than in main().

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit 36b8d179a5ae4a3c2a27a869bcbfd535d7d4a2e0
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sat Oct 3 21:20:23 2009 +0200

    modprobe: don't modify the modname string passed to do_modprobe

    The function will work on a copy instead.

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit b8fef87bd628dfaf0f1e43420be1ed4e49f9d35b
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sat Oct 3 21:30:18 2009 +0200

    modprobe: add softdep command

    Imlementation notes
    -------------------

    * find_softdep()/do_softdep() mirrors find_command()/do_command()
      precisely. (And if they don't, that's a bug.)

    * Failures in the indirect modprobes, ie. those run by the softdep
      command, are ignored. This seems correct for removing, but I'm
      unsure what the proper action is when installing.

    An example - or how it's supposed to work
    -----------------------------------------

    Configuration:
    softdep foo --pre pre1 pre2 --post post1 post2

    Installing a module:

    $modprobe foo <CMDLINE_OPTS>

    yields

    modprobe pre1
    modprobe pre2
    modprobe --ignore-install foo <CMDLINE_OPTS>
    modprobe post1
    modprobe post2

    Likewiese, removing a module:

    $modprobe -r foo

    yields

    modprobe -r post2
    modprobe -r post1
    modprobe --ignore-remove -r foo
    modprobe -r pre2
    modprobe -r pre1

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit d7a8758609dc13d048a249295c2dcc4345cbf40f
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sun Oct 4 17:00:43 2009 +0200

    elfops: remove errfn_t from load_strings

    Commit 528db92ab1dd0d75dba415b9f3dc81f5a34773ce added an errfn_t
    parameter to elfops_core.c:load_strings. This was for the purpose
    of detecting missing terminators at the end of ELF-sections with
    strings in them, such as .modinfo.

    However, the committer (that'd be me) forgot to add any actual code to
    load_strings() and now the errfn_t parameter complicates the error
    handling when softdep is used.

    This commit removes that parameter and adds a non-fatal warning message.

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>

commit f2150e81dbeb547f9fc70b8c622c38cc6f987a50
Author: Andreas Robinson <andr345@xxxxxxxxx>
Date:   Sun Oct 4 17:18:38 2009 +0200

    modprobe: add simple softdep loop detector

    Signed-off-by: Andreas Robinson <andr345@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux