[PATCH 0/4] depmod and modprobe error message fixups

[PATCH 0/4] depmod and modprobe error message fixups

[Laura Abbott]

While doing some experimental work with kernel packaging,
I found a few places where a lack of error messages made
it difficult to figure out what was going on. This cleans
up the error messages to be a bit more useful.

Laura Abbott (4):
  build: Properly check for Cython
  modprobe: Add appropriate error message when path is missing
  depmod: Fix message printing before log_setup_kmod_log
  depmod: Add error message for bad version

 configure.ac     |  5 ++++-
 tools/depmod.c   | 15 +++++++++------
 tools/modprobe.c |  5 ++++-
 3 files changed, 17 insertions(+), 8 deletions(-)

-- 
2.4.3

[PATCH 1/4] build: Properly check for Cython

[Laura Abbott]

Cython is necessary to compile if --enable-python is used.
Currently, the configuration just sets Cython to ':' if
it isn't found. ':' is a valid command which results in
confusing build errors:

  CCLD     libkmod/libkmod.la
  CCLD     libkmod/libkmod-internal.la
ar: `u' modifier ignored since `D' is the default (see `U')
  CYTHON  libkmod/python/kmod/kmod.c
  CC       libkmod/python/kmod/libkmod_python_kmod_kmod_la-kmod.lo
gcc: error: ./libkmod/python/kmod/kmod.c: No such file or directory
gcc: fatal error: no input files

Explicitly check if cython is available and then error out
if it isn't found.
---
 configure.ac | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index d4f84bd..1648a17 100644
--- a/configure.ac
+++ b/configure.ac
@@ -162,7 +162,10 @@ AC_ARG_ENABLE([python],
 	[], [enable_python=no])
 AS_IF([test "x$enable_python" = "xyes"], [
 	AM_PATH_PYTHON(,,[:])
-	AC_PATH_PROG([CYTHON], [cython], [:])
+	AC_PATH_PROG([CYTHON], [cython], [no])
+	if test x"$CYTHON" == x"no"; then
+		AC_MSG_ERROR([Please install Cython before building])
+	fi
 
 	PKG_CHECK_MODULES([PYTHON], [python-${PYTHON_VERSION}],
 			  [have_python=yes],
-- 
2.4.3

[PATCH 2/4] modprobe: Add appropriate error message when path is missing

[Laura Abbott]

Currently, modprobe fails with no output by default if the
search paths it tries are missing:

$ modprobe -S notakernel dm-crypt
$
$ modprobe -S notakernel lkjjweiojo
$

This is fairly cryptic and not at all obvious there is a problem
unless the error code is checked or verbose flags are used.
Add a message to give a better idea that something went wrong.
---
 tools/modprobe.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3ba8f52..b50a385 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -489,8 +489,11 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
 						const char *options) = NULL;
 
 	err = kmod_module_new_from_lookup(ctx, alias, &list);
-	if (err < 0)
+	if (err < 0) {
+		ERR("Could not generate list of modules from context\n");
+		ERR("(Is your version correct)?)\n");
 		return err;
+	}
 
 	if (list == NULL) {
 		LOG("Module %s not found.\n", alias);
-- 
2.4.3

[PATCH 3/4] depmod: Fix message printing before log_setup_kmod_log

[Laura Abbott]

Until log_setup_kmod_log is called, the only messages that
can be printed are the default level. Bump a few deprecated
messages to ERR to ensure they get printed and drop some DBG
prints that will never occur unless the compiled default is DBG.
---
 tools/depmod.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tools/depmod.c b/tools/depmod.c
index 2a08b6e..30f6191 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2455,10 +2455,10 @@ static int do_depmod(int argc, char *argv[])
 		case 'r':
 		case 'm':
 			if (idx > 0)
-				WRN("Ignored deprecated option --%s\n",
+				ERR("Ignored deprecated option --%s\n",
 				    cmdopts[idx].name);
 			else
-				WRN("Ignored deprecated option -%c\n", c);
+				ERR("Ignored deprecated option -%c\n", c);
 
 			break;
 		case 'h':
@@ -2498,11 +2498,9 @@ static int do_depmod(int argc, char *argv[])
 		if (out == stdout)
 			goto done;
 		/* ignore up-to-date errors (< 0) */
-		if (depfile_up_to_date(cfg.dirname) == 1) {
-			DBG("%s/modules.dep is up to date!\n", cfg.dirname);
+		if (depfile_up_to_date(cfg.dirname) == 1)
 			goto done;
-		}
-		DBG("%s/modules.dep is outdated, do -a\n", cfg.dirname);
+
 		all = 1;
 	}
 
-- 
2.4.3

[PATCH 4/4] depmod: Add error message for bad version

[Laura Abbott]

Currently, if a value that doesn't match a kernel version
("%u.%u") is passed in, depmod silently falls back to
using uname. Make it clear to the user that this is happening
by giving a message to the user.
---
 tools/depmod.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/depmod.c b/tools/depmod.c
index 30f6191..f491542 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -2480,6 +2480,11 @@ static int do_depmod(int argc, char *argv[])
 		cfg.kversion = argv[optind];
 		optind++;
 	} else {
+		if (optind < argc) {
+			ERR("Bad version passed %s\n", argv[optind]);
+			ERR("Falling back to uname\n");
+		}
+
 		if (uname(&un) < 0) {
 			CRIT("uname() failed: %s\n", strerror(errno));
 			goto cmdline_failed;
-- 
2.4.3

Re: [PATCH 1/4] build: Properly check for Cython

[Lucas De Marchi]

On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>
> Cython is necessary to compile if --enable-python is used.
> Currently, the configuration just sets Cython to ':' if
> it isn't found. ':' is a valid command which results in
> confusing build errors:
>
>   CCLD     libkmod/libkmod.la
>   CCLD     libkmod/libkmod-internal.la
> ar: `u' modifier ignored since `D' is the default (see `U')
>   CYTHON  libkmod/python/kmod/kmod.c
>   CC       libkmod/python/kmod/libkmod_python_kmod_kmod_la-kmod.lo
> gcc: error: ./libkmod/python/kmod/kmod.c: No such file or directory
> gcc: fatal error: no input files
>
> Explicitly check if cython is available and then error out
> if it isn't found.
> ---

Since we distribute the  generated python files, cython is only needed
on "building from git tree" cases, not for packages.


Lucas De Marchi

Re: [PATCH 2/4] modprobe: Add appropriate error message when path is missing

[Lucas De Marchi]

On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>
> Currently, modprobe fails with no output by default if the
> search paths it tries are missing:
>
> $ modprobe -S notakernel dm-crypt
> $
> $ modprobe -S notakernel lkjjweiojo
> $
>
> This is fairly cryptic and not at all obvious there is a problem
> unless the error code is checked or verbose flags are used.
> Add a message to give a better idea that something went wrong.
> ---
>  tools/modprobe.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/modprobe.c b/tools/modprobe.c
> index 3ba8f52..b50a385 100644
> --- a/tools/modprobe.c
> +++ b/tools/modprobe.c
> @@ -489,8 +489,11 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
>                                                 const char *options) = NULL;
>
>         err = kmod_module_new_from_lookup(ctx, alias, &list);
> -       if (err < 0)
> +       if (err < 0) {
> +               ERR("Could not generate list of modules from context\n");
> +               ERR("(Is your version correct)?)\n");

I think we should not ask "is your version correct?", but rather give
information for the user to figure it out. So... maybe export
kmod_get_dirname() from the library and here do something like:

err = kmod_module_new_from_lookup(ctx, alias, &list);
if (err < 0) {
        ERR("Could not lookup module '%s' from module directory '%s'\n",
            alias, kmod_get_dirname(ctx));
        return err;
}

Actually this case is very similar to the check "list == NULL" below
this and maybe we should merge them with the same log message. What do
you think?


Lucas De Marchi

Re: [PATCH 3/4] depmod: Fix message printing before log_setup_kmod_log

[Lucas De Marchi]

On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>
> Until log_setup_kmod_log is called, the only messages that
> can be printed are the default level. Bump a few deprecated
> messages to ERR to ensure they get printed and drop some DBG
> prints that will never occur unless the compiled default is DBG.
> ---
>  tools/depmod.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/tools/depmod.c b/tools/depmod.c
> index 2a08b6e..30f6191 100644
> --- a/tools/depmod.c
> +++ b/tools/depmod.c
> @@ -2455,10 +2455,10 @@ static int do_depmod(int argc, char *argv[])
>                 case 'r':
>                 case 'm':
>                         if (idx > 0)
> -                               WRN("Ignored deprecated option --%s\n",
> +                               ERR("Ignored deprecated option --%s\n",
>                                     cmdopts[idx].name);
>                         else
> -                               WRN("Ignored deprecated option -%c\n", c);
> +                               ERR("Ignored deprecated option -%c\n", c);

/me confused. The default priority for depmod is LOG_WARNING

>                         break;
>                 case 'h':
> @@ -2498,11 +2498,9 @@ static int do_depmod(int argc, char *argv[])
>                 if (out == stdout)
>                         goto done;
>                 /* ignore up-to-date errors (< 0) */
> -               if (depfile_up_to_date(cfg.dirname) == 1) {
> -                       DBG("%s/modules.dep is up to date!\n", cfg.dirname);
> +               if (depfile_up_to_date(cfg.dirname) == 1)
>                         goto done;
> -               }
> -               DBG("%s/modules.dep is outdated, do -a\n", cfg.dirname);
> +

I don't mind losing these log messages. I guess they were useful in
the beginning but they aren't anymore.


Lucas De Marchi


>                 all = 1;
>         }
>
> --
> 2.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-modules" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Lucas De Marchi

Re: [PATCH 4/4] depmod: Add error message for bad version

[Lucas De Marchi]

On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>
> Currently, if a value that doesn't match a kernel version
> ("%u.%u") is passed in, depmod silently falls back to
> using uname. Make it clear to the user that this is happening
> by giving a message to the user.
> ---

Actually I think we should not fallback. If the user passed a wrong
number, fallbacking to the currently running kernel is plain wrong.
This was done mostly due to compatibility with module-init-tools but I
prefer breaking it rather than silently (or verbose as per your patch)
doing the wrong thing.

thanks

Lucas De Marchi

Re: [PATCH 0/4] depmod and modprobe error message fixups

[Lucas De Marchi]

Hi Laura,

CC'ing Lennart and Kay

On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>
> While doing some experimental work with kernel packaging,
> I found a few places where a lack of error messages made
> it difficult to figure out what was going on. This cleans
> up the error messages to be a bit more useful.
>
> Laura Abbott (4):
>   build: Properly check for Cython
>   modprobe: Add appropriate error message when path is missing
>   depmod: Fix message printing before log_setup_kmod_log
>   depmod: Add error message for bad version

Thanks very much for the patches. I commented on each of them
individually.  Could you send a v2 with the comments addressed?

For the cython patch, I'm not sure. I don't want the cython dependency
to be a one off since there are other things we ship prebuilt in
packages like the man pages. Maybe just stop shipping prebuilt stuff
in the distributed file like systemd did would be ok. Lennart, Kay,
did you had any problems with distributions due to this?


Lucas De Marchi

Re: [PATCH 0/4] depmod and modprobe error message fixups

[Kay Sievers]

On Sat, Sep 12, 2015 at 8:05 PM, Lucas De Marchi
<lucas.de.marchi@gmail.com> wrote:
> Hi Laura,
>
> CC'ing Lennart and Kay
>
> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>>
>> While doing some experimental work with kernel packaging,
>> I found a few places where a lack of error messages made
>> it difficult to figure out what was going on. This cleans
>> up the error messages to be a bit more useful.
>>
>> Laura Abbott (4):
>>   build: Properly check for Cython
>>   modprobe: Add appropriate error message when path is missing
>>   depmod: Fix message printing before log_setup_kmod_log
>>   depmod: Add error message for bad version
>
> Thanks very much for the patches. I commented on each of them
> individually.  Could you send a v2 with the comments addressed?
>
> For the cython patch, I'm not sure. I don't want the cython dependency
> to be a one off since there are other things we ship prebuilt in
> packages like the man pages. Maybe just stop shipping prebuilt stuff
> in the distributed file like systemd did would be ok. Lennart, Kay,
> did you had any problems with distributions due to this?

No known problems I know of. We will not got back to pre-created content..

Kay

Re: [PATCH 1/4] build: Properly check for Cython

[Laura Abbott]

On 09/12/2015 11:27 AM, Lucas De Marchi wrote:
> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>>
>> Cython is necessary to compile if --enable-python is used.
>> Currently, the configuration just sets Cython to ':' if
>> it isn't found. ':' is a valid command which results in
>> confusing build errors:
>>
>>    CCLD     libkmod/libkmod.la
>>    CCLD     libkmod/libkmod-internal.la
>> ar: `u' modifier ignored since `D' is the default (see `U')
>>    CYTHON  libkmod/python/kmod/kmod.c
>>    CC       libkmod/python/kmod/libkmod_python_kmod_kmod_la-kmod.lo
>> gcc: error: ./libkmod/python/kmod/kmod.c: No such file or directory
>> gcc: fatal error: no input files
>>
>> Explicitly check if cython is available and then error out
>> if it isn't found.
>> ---
>
> Since we distribute the  generated python files, cython is only needed
> on "building from git tree" cases, not for packages.
>
>
> Lucas De Marchi
>

I don't think I have a clear answer about whether or not you want to take
this patch even after Kay's response. If you don't want to add the
autoconf check I'll update the README and put a note under hacking to make
sure cython is installed if the intention is to ship the generated files.

Thanks,
Laura

Re: [PATCH 3/4] depmod: Fix message printing before log_setup_kmod_log

[Laura Abbott]

On 09/12/2015 11:55 AM, Lucas De Marchi wrote:
> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>>
>> Until log_setup_kmod_log is called, the only messages that
>> can be printed are the default level. Bump a few deprecated
>> messages to ERR to ensure they get printed and drop some DBG
>> prints that will never occur unless the compiled default is DBG.
>> ---
>>   tools/depmod.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/depmod.c b/tools/depmod.c
>> index 2a08b6e..30f6191 100644
>> --- a/tools/depmod.c
>> +++ b/tools/depmod.c
>> @@ -2455,10 +2455,10 @@ static int do_depmod(int argc, char *argv[])
>>                  case 'r':
>>                  case 'm':
>>                          if (idx > 0)
>> -                               WRN("Ignored deprecated option --%s\n",
>> +                               ERR("Ignored deprecated option --%s\n",
>>                                      cmdopts[idx].name);
>>                          else
>> -                               WRN("Ignored deprecated option -%c\n", c);
>> +                               ERR("Ignored deprecated option -%c\n", c);
>
> /me confused. The default priority for depmod is LOG_WARNING
>

Right but it isn't set until you call log_setup_kmod_log. Until then it
uses the global default which is currently LOG_ERR so anything at a lower
level before this call is mostly useless. Changing the global default in
tools/log.c to LOG_WARN might be another option.

Thanks,
Laura

Re: [PATCH 4/4] depmod: Add error message for bad version

[Laura Abbott]

On 09/12/2015 12:00 PM, Lucas De Marchi wrote:
> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org> wrote:
>>
>> Currently, if a value that doesn't match a kernel version
>> ("%u.%u") is passed in, depmod silently falls back to
>> using uname. Make it clear to the user that this is happening
>> by giving a message to the user.
>> ---
>
> Actually I think we should not fallback. If the user passed a wrong
> number, fallbacking to the currently running kernel is plain wrong.
> This was done mostly due to compatibility with module-init-tools but I
> prefer breaking it rather than silently (or verbose as per your patch)
> doing the wrong thing.
>

Sounds fine to me if you are okay with the breakage.

Thanks,
Laura

Re: [PATCH 1/4] build: Properly check for Cython

[Lucas De Marchi]

On Fri, Sep 18, 2015 at 2:38 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 09/12/2015 11:27 AM, Lucas De Marchi wrote:
>>
>> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org>
>> wrote:
>>>
>>>
>>> Cython is necessary to compile if --enable-python is used.
>>> Currently, the configuration just sets Cython to ':' if
>>> it isn't found. ':' is a valid command which results in
>>> confusing build errors:
>>>
>>>    CCLD     libkmod/libkmod.la
>>>    CCLD     libkmod/libkmod-internal.la
>>> ar: `u' modifier ignored since `D' is the default (see `U')
>>>    CYTHON  libkmod/python/kmod/kmod.c
>>>    CC       libkmod/python/kmod/libkmod_python_kmod_kmod_la-kmod.lo
>>> gcc: error: ./libkmod/python/kmod/kmod.c: No such file or directory
>>> gcc: fatal error: no input files
>>>
>>> Explicitly check if cython is available and then error out
>>> if it isn't found.
>>> ---
>>
>>
>> Since we distribute the  generated python files, cython is only needed
>> on "building from git tree" cases, not for packages.
>>
>>
>> Lucas De Marchi
>>
>
> I don't think I have a clear answer about whether or not you want to take
> this patch even after Kay's response. If you don't want to add the
> autoconf check I'll update the README and put a note under hacking to make
> sure cython is installed if the intention is to ship the generated files.

Either update the README or remove everything from the build system
that makes the dependencies different for building from git and
building from package. Also make sure we don't ship prebuilt stuff
anymore.  I'll accept any of those approaches.

thanks

Lucas De Marchi

Re: [PATCH 3/4] depmod: Fix message printing before log_setup_kmod_log

[Lucas De Marchi]

On Fri, Sep 18, 2015 at 3:26 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 09/12/2015 11:55 AM, Lucas De Marchi wrote:
>>
>> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org>
>> wrote:
>>>
>>>
>>> Until log_setup_kmod_log is called, the only messages that
>>> can be printed are the default level. Bump a few deprecated
>>> messages to ERR to ensure they get printed and drop some DBG
>>> prints that will never occur unless the compiled default is DBG.
>>> ---
>>>   tools/depmod.c | 10 ++++------
>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/depmod.c b/tools/depmod.c
>>> index 2a08b6e..30f6191 100644
>>> --- a/tools/depmod.c
>>> +++ b/tools/depmod.c
>>> @@ -2455,10 +2455,10 @@ static int do_depmod(int argc, char *argv[])
>>>                  case 'r':
>>>                  case 'm':
>>>                          if (idx > 0)
>>> -                               WRN("Ignored deprecated option --%s\n",
>>> +                               ERR("Ignored deprecated option --%s\n",
>>>                                      cmdopts[idx].name);
>>>                          else
>>> -                               WRN("Ignored deprecated option -%c\n",
>>> c);
>>> +                               ERR("Ignored deprecated option -%c\n",
>>> c);
>>
>>
>> /me confused. The default priority for depmod is LOG_WARNING
>>
>
> Right but it isn't set until you call log_setup_kmod_log. Until then it
> uses the global default which is currently LOG_ERR so anything at a lower
> level before this call is mostly useless. Changing the global default in
> tools/log.c to LOG_WARN might be another option.

Yeah, you are right. I'd prefer to change the global default to
LOG_WARN for all tools.  This will only survive until each tool sets
its own log level.  The DBG can indeed be removed.

-- 
Lucas De Marchi

Re: [PATCH 4/4] depmod: Add error message for bad version

[Lucas De Marchi]

On Fri, Sep 18, 2015 at 3:30 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 09/12/2015 12:00 PM, Lucas De Marchi wrote:
>>
>> On Fri, Sep 11, 2015 at 5:55 PM, Laura Abbott <labbott@fedoraproject.org>
>> wrote:
>>>
>>>
>>> Currently, if a value that doesn't match a kernel version
>>> ("%u.%u") is passed in, depmod silently falls back to
>>> using uname. Make it clear to the user that this is happening
>>> by giving a message to the user.
>>> ---
>>
>>
>> Actually I think we should not fallback. If the user passed a wrong
>> number, fallbacking to the currently running kernel is plain wrong.
>> This was done mostly due to compatibility with module-init-tools but I
>> prefer breaking it rather than silently (or verbose as per your patch)
>> doing the wrong thing.
>>
>
> Sounds fine to me if you are okay with the breakage.

yep

thanks
Lucas De Marchi