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-hotplug" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
