On Thu, 2009-05-21 at 19:19 +0100, Alan Jenkins wrote: > On 5/21/09, Andreas Robinson <[email protected]> wrote: > > On Thu, 2009-05-21 at 12:33 +0100, Alan Jenkins wrote: > >> On 5/21/09, Andreas Robinson <[email protected]> wrote: > >> > > >> > error("Could not read '%s': %s\n", > >> > mod->filename, strerror(errno)); > >> > >> > >> This will change the error message to something less helpful. You > >> should probably use insert_moderror() instead of strerror(). > > > > I'm not so sure about that. strerror(ENOEXEC) == "Exec format error", > > and while I agree it is less clear than "invalid module format", it is > > exactly the right message for this kind of error. I.e we're trying to > > load an executable but it really isn't => Exec format error. It is > > simply seen less often than say, "File not found". > > Yeah, I didn't say it was actually wrong. > > > To use insert_moderror(), the message would have to be formatted like > > this: > > > > error("Could not read '%s': %s\n", mod->filename, > > (errno == ENOEXEC) ? insert_moderror(errno) : strerror(errno)); > > > > Though this is probably better: > > > > error("Could not read '%s': %s\n", mod->filename, > > (errno == ENOEXEC) ? "invalid module format" : strerror(errno)); > > > > since insert_moderror() is written explicitly for init_module(). > > > > If it isn't done like that, we would get insert_moderror(ENOENT) == > > "Unknown symbol in module, or unknown parameter (see dmesg)" which > > clearly isn't the "File not found" we're expecting. > > Agreed, that would be very wrong. The second version above looks good.
And, here's the reworked version. >From d1877dc6e4bbc5344990cc6df6a35cc45f246788 Mon Sep 17 00:00:00 2001 From: Andreas Robinson <[email protected]> Date: Thu, 21 May 2009 22:29:52 +0200 Subject: [PATCH] modprobe, tests: make modprobe fail noisily on non-ELF files Also remove fake placeholder modules in the test suite e.g "echo Test > foo.ko && modprobe foo". They don't work anymore. Signed-off-by: Andreas Robinson <[email protected]> --- modprobe.c | 21 +++------------------ tests/test-modprobe-indexed/10alias.sh | 9 +++++---- tests/test-modprobe/10alias.sh | 9 +++++---- tests/test-modprobe/26blacklist.sh | 7 ++++--- 4 files changed, 17 insertions(+), 29 deletions(-) diff --git a/modprobe.c b/modprobe.c index a3cdbf6..f84663f 100644 --- a/modprobe.c +++ b/modprobe.c @@ -666,23 +666,9 @@ static int insmod(struct list_head *list, module = grab_elf_file_fd(mod->filename, fd); if (!module) { - /* This is an ugly hack that maintains the logic where - * init_module() sets errno = ENOEXEC if the file is - * not an ELF object. - */ - if (errno == ENOEXEC) { - struct stat st; - optstring = add_extra_options(mod->modname, - optstring, options); - if (dry_run) - goto out; - fstat(fd, &st); - ret = init_module(NULL, st.st_size, optstring); - goto out_hack; - } - - error("Could not read '%s': %s\n", - mod->filename, strerror(errno)); + error("Could not read '%s': %s\n", mod->filename, + (errno == ENOEXEC) ? "Invalid module format" : + strerror(errno)); goto out_unlock; } if (newname) @@ -701,7 +687,6 @@ static int insmod(struct list_head *list, goto out; ret = init_module(module->data, module->len, optstring); -out_hack: if (ret != 0) { if (errno == EEXIST) { if (first_time) diff --git a/tests/test-modprobe-indexed/10alias.sh b/tests/test-modprobe-indexed/10alias.sh index bd16131..c2a2f5a 100644 --- a/tests/test-modprobe-indexed/10alias.sh +++ b/tests/test-modprobe-indexed/10alias.sh @@ -21,7 +21,8 @@ rm -f $MODULE_DIR/modules.alias rm -f $MODULE_DIR/modules.alias.bin rm -f tests/tmp/etc/modprobe.d/modprobe.conf -echo Test > $MODULE_DIR/kernel/foo.ko +cp tests/data/$BITNESS/complex/complex_a-$BITNESS.ko $MODULE_DIR/kernel/foo.ko +SIZE2=`wc -c < $MODULE_DIR/kernel/foo.ko` # Shouldn't complain if can't open modules.alias [ "`modprobe bar 2>&1`" = "FATAL: Module bar not found." ] @@ -34,12 +35,12 @@ modindex -o $MODULE_DIR/modules.alias.bin < $MODULE_DIR/modules.alias.bin.temp # Normal alias should override it. mkdir -p tests/tmp/etc/modprobe.d echo 'alias bar foo' > tests/tmp/etc/modprobe.d/modprobe.conf -[ "`modprobe foo 2>&1`" = "INIT_MODULE: 5 " ] +[ "`modprobe foo 2>&1`" = "INIT_MODULE: $SIZE2 " ] # If there's a real module, alias from modules.alias must NOT override. echo "foo alias_$BITNESS" > $MODULE_DIR/modules.alias.bin.temp modindex -o $MODULE_DIR/modules.alias.bin < $MODULE_DIR/modules.alias.bin.temp -[ "`modprobe foo 2>&1`" = "INIT_MODULE: 5 " ] +[ "`modprobe foo 2>&1`" = "INIT_MODULE: $SIZE2 " ] # If there's an install command, modules.alias must not override. echo 'install bar echo foo' > tests/tmp/etc/modprobe.d/modprobe.conf @@ -60,5 +61,5 @@ modindex -o $MODULE_DIR/modules.alias.bin < $MODULE_DIR/modules.alias.bin.temp OUT="`modprobe bar 2>&1`" [ "$OUT" = "INIT_MODULE: $SIZE option2 option1 -INIT_MODULE: 5 option1" ] || [ "$OUT" = "INIT_MODULE: 5 option1 +INIT_MODULE: $SIZE2 option1" ] || [ "$OUT" = "INIT_MODULE: $SIZE2 option1 INIT_MODULE: $SIZE option2 option1" ] diff --git a/tests/test-modprobe/10alias.sh b/tests/test-modprobe/10alias.sh index ebe2ea0..c5abf43 100755 --- a/tests/test-modprobe/10alias.sh +++ b/tests/test-modprobe/10alias.sh @@ -16,7 +16,8 @@ SIZE=`wc -c < tests/data/$BITNESS/alias/alias-$BITNESS.ko` echo "/lib/modules/$MODTEST_UNAME/kernel/alias-$BITNESS.ko:" > $MODULE_DIR/modules.dep echo "/lib/modules/$MODTEST_UNAME/kernel/foo.ko:" >> $MODULE_DIR/modules.dep -echo Test > $MODULE_DIR/kernel/foo.ko +cp tests/data/$BITNESS/complex/complex_a-$BITNESS.ko $MODULE_DIR/kernel/foo.ko +SIZE2=`wc -c < $MODULE_DIR/kernel/foo.ko` # Shouldn't complain if can't open modules.alias [ "`modprobe bar 2>&1`" = "FATAL: Module bar not found." ] @@ -28,11 +29,11 @@ echo "alias bar alias-$BITNESS" > $MODULE_DIR/modules.alias # Normal alias should override it. mkdir -p tests/tmp/etc/modprobe.d echo 'alias bar foo' > tests/tmp/etc/modprobe.d/modprobe.conf -[ "`modprobe foo 2>&1`" = "INIT_MODULE: 5 " ] +[ "`modprobe foo 2>&1`" = "INIT_MODULE: $SIZE2 " ] # If there's a real module, alias from modules.alias must NOT override. echo "alias foo alias-$BITNESS" > $MODULE_DIR/modules.alias -[ "`modprobe foo 2>&1`" = "INIT_MODULE: 5 " ] +[ "`modprobe foo 2>&1`" = "INIT_MODULE: $SIZE2 " ] # If there's an install command, modules.alias must not override. echo 'install bar echo foo' > tests/tmp/etc/modprobe.d/modprobe.conf @@ -51,5 +52,5 @@ echo "alias bar foo" >> $MODULE_DIR/modules.alias OUT="`modprobe bar 2>&1`" [ "$OUT" = "INIT_MODULE: $SIZE option2 option1 -INIT_MODULE: 5 option1" ] || [ "$OUT" = "INIT_MODULE: 5 option1 +INIT_MODULE: $SIZE2 option1" ] || [ "$OUT" = "INIT_MODULE: $SIZE2 option1 INIT_MODULE: $SIZE option2 option1" ] diff --git a/tests/test-modprobe/26blacklist.sh b/tests/test-modprobe/26blacklist.sh index 8fcff39..482ee93 100755 --- a/tests/test-modprobe/26blacklist.sh +++ b/tests/test-modprobe/26blacklist.sh @@ -15,7 +15,8 @@ SIZE=`wc -c < tests/data/$BITNESS/alias/alias-$BITNESS.ko` echo "/lib/modules/$MODTEST_UNAME/kernel/alias-$BITNESS.ko:" > $MODULE_DIR/modules.dep echo "/lib/modules/$MODTEST_UNAME/kernel/foo.ko:" >> $MODULE_DIR/modules.dep -echo Test > $MODULE_DIR/kernel/foo.ko +cp tests/data/$BITNESS/complex/complex_a-$BITNESS.ko $MODULE_DIR/kernel/foo.ko +SIZE2=`wc -c < $MODULE_DIR/kernel/foo.ko` # First, alias found in modules.alias works. echo "alias bar alias-$BITNESS" > $MODULE_DIR/modules.alias @@ -28,7 +29,7 @@ echo "blacklist alias-$BITNESS" > tests/tmp/etc/modprobe.d/modprobe.conf # Blacklist doesn't effect other aliases. echo "alias bar foo" >> $MODULE_DIR/modules.alias -[ "`modprobe bar 2>&1`" = "INIT_MODULE: 5 " ] +[ "`modprobe bar 2>&1`" = "INIT_MODULE: $SIZE2 " ] # Blacklist both. echo "blacklist foo" >> tests/tmp/etc/modprobe.d/modprobe.conf @@ -38,5 +39,5 @@ echo "blacklist foo" >> tests/tmp/etc/modprobe.d/modprobe.conf rm -f tests/tmp/etc/modprobe.d/modprobe.conf RESULT="`modprobe bar 2>&1`" [ "$RESULT" = "INIT_MODULE: $SIZE -INIT_MODULE: 5 " ] || [ "$RESULT" = "INIT_MODULE: 5 +INIT_MODULE: $SIZE2 " ] || [ "$RESULT" = "INIT_MODULE: $SIZE2 INIT_MODULE: $SIZE " ] -- 1.6.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-modules" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
