[PATCH] modinfo: dont print module name when --field is given

[PATCH] modinfo: dont print module name when --field is given

[Xaver Hörl]

I hope I found the right mailing list for this issue, which
came up here: https://github.com/NixOS/nixpkgs/pull/96008

The man page for modinfo claims that with the -F / --field option only
the specified field gets printed. Currently this is not true as for builtin modules, the name is always printed.

Try something like "modinfo -F firmware unix" for example.
It will print: "name:           unix"

So either the man page needs to be updated or the behaviour changed.
The following patch should fix this:

---
 tools/modinfo.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/modinfo.c b/tools/modinfo.c
index 0231bb0..7b2259e 100644
--- a/tools/modinfo.c
+++ b/tools/modinfo.c
@@ -178,7 +178,10 @@ static int modinfo_do(struct kmod_module *mod)
 	is_builtin = (filename == NULL);

 	if (is_builtin) {
-		printf("%-16s%s%c", "name:", kmod_module_get_name(mod), separator);
+		if (field == NULL || field != NULL && streq(field, "name")){
+			printf("%-16s%s%c", "name:",
+			       kmod_module_get_name(mod), separator);
+		}
 		filename = "(builtin)";
 	}

--
2.28.0

Re: [PATCH] modinfo: dont print module name when --field is given

[Lucas De Marchi]

On Sun, Aug 23, 2020 at 3:01 PM Xaver Hörl <hoe.dom@gmx.de> wrote:
>
> I hope I found the right mailing list for this issue, which
> came up here: https://github.com/NixOS/nixpkgs/pull/96008
>
> The man page for modinfo claims that with the -F / --field option only
> the specified field gets printed. Currently this is not true as for builtin modules, the name is always printed.
>
> Try something like "modinfo -F firmware unix" for example.
> It will print: "name:           unix"
>
> So either the man page needs to be updated or the behaviour changed.

yeah, it seems something to change in the code. This behavior
started when we added support in modinfo to show information
about builtin modules.

> The following patch should fix this:
>
> ---
>  tools/modinfo.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/modinfo.c b/tools/modinfo.c
> index 0231bb0..7b2259e 100644
> --- a/tools/modinfo.c
> +++ b/tools/modinfo.c
> @@ -178,7 +178,10 @@ static int modinfo_do(struct kmod_module *mod)
>         is_builtin = (filename == NULL);
>
>         if (is_builtin) {
> -               printf("%-16s%s%c", "name:", kmod_module_get_name(mod), separator);
> +               if (field == NULL || field != NULL && streq(field, "name")){

the name would be handled by the next if/else branch. Unconditionally printing
the name at the beginning for built-in modules doesn't seem something important
to retain. I'd rather handle it in the `if (is_builtin && err ==
-ENOENT) {` below just
not to exit without printing anything. In that case, printing both
name and filename
would be preferred, so the user knows why there isn't additional information

thanks
Lucas De Marchi

> +                       printf("%-16s%s%c", "name:",
> +                              kmod_module_get_name(mod), separator);
> +               }
>                 filename = "(builtin)";
>         }
>
> --
> 2.28.0
>

[PATCH] modinfo: dont print module name when --field is given

[Xaver Hörl]

On Tue, Aug 25, 2020 at 10:27:46AM -0700, Lucas De Marchi wrote:
> the name would be handled by the next if/else branch. Unconditionally printing
> the name at the beginning for built-in modules doesn't seem something important
> to retain. I'd rather handle it in the `if (is_builtin && err ==
> -ENOENT) {` below just
> not to exit without printing anything. In that case, printing both
> name and filename
> would be preferred, so the user knows why there isn't additional information

Dropping the "name:" part with --field seems the right thing (updated
patch below implements this). But I don't know what exactly to do about
the error case, not sure if I get your intention right there.

If --field is not given, the name and filename (i.e. "(builtin)" in
this case) will be there anyway, on the other hand if --field
_is_ given it seems a bit inconsistent to give additional output.
Unless you consider this case an error condition of sorts, but the
return value being 0 suggests otherwise.


Thanks for having a look at this!
Xaver Hörl

---
 tools/modinfo.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/modinfo.c b/tools/modinfo.c
index 0231bb0..b49f622 100644
--- a/tools/modinfo.c
+++ b/tools/modinfo.c
@@ -178,14 +178,21 @@ static int modinfo_do(struct kmod_module *mod)
 	is_builtin = (filename == NULL);

 	if (is_builtin) {
-		printf("%-16s%s%c", "name:", kmod_module_get_name(mod), separator);
 		filename = "(builtin)";
 	}

-	if (field != NULL && streq(field, "filename")) {
+	if (field != NULL && streq(field, "name")) {
+		printf("%s%c", kmod_module_get_name(mod),
+			       separator);
+		return 0;
+	} else if (field != NULL && streq(field, "filename")) {
 		printf("%s%c", filename, separator);
 		return 0;
 	} else if (field == NULL) {
+		if (is_builtin) {
+			printf("%-16s%s%c", "name:",
+			       kmod_module_get_name(mod), separator);
+		}
 		printf("%-16s%s%c", "filename:",
 		       filename, separator);
 	}
--
2.28.0

Re: [PATCH] modinfo: dont print module name when --field is given

[Lucas De Marchi]

On Wed, Aug 26, 2020 at 2:36 AM Xaver Hörl <hoe.dom@gmx.de> wrote:
>
> On Tue, Aug 25, 2020 at 10:27:46AM -0700, Lucas De Marchi wrote:
> > the name would be handled by the next if/else branch. Unconditionally printing
> > the name at the beginning for built-in modules doesn't seem something important
> > to retain. I'd rather handle it in the `if (is_builtin && err ==
> > -ENOENT) {` below just
> > not to exit without printing anything. In that case, printing both
> > name and filename
> > would be preferred, so the user knows why there isn't additional information
>
> Dropping the "name:" part with --field seems the right thing (updated
> patch below implements this). But I don't know what exactly to do about
> the error case, not sure if I get your intention right there.
>
> If --field is not given, the name and filename (i.e. "(builtin)" in
> this case) will be there anyway, on the other hand if --field

I was thinking about this specific error condition:
                if (is_builtin && err == -ENOENT) {
                        /*
                         * This is an old kernel that does not have a file
                         * with information about built-in modules.
                         */
                        return 0;
                }

I.e. if --field is not given and the kernel is old enough not to contain
/lib/modules/$(uname -r)/modules.builtin.modinfo, then we print
name + filename (builtin).

$ modinfo unix
name:           unix
filename:       (builtin)
alias:          net-pf-1
license:        GPL
file:           net/unix/unix
$ sudo mv ll
$ modinfo unix
name:           unix
filename:       (builtin)


Lucas De Marchi

> _is_ given it seems a bit inconsistent to give additional output.
> Unless you consider this case an error condition of sorts, but the
> return value being 0 suggests otherwise.
>
>
> Thanks for having a look at this!
> Xaver Hörl
>
> ---
>  tools/modinfo.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/modinfo.c b/tools/modinfo.c
> index 0231bb0..b49f622 100644
> --- a/tools/modinfo.c
> +++ b/tools/modinfo.c
> @@ -178,14 +178,21 @@ static int modinfo_do(struct kmod_module *mod)
>         is_builtin = (filename == NULL);
>
>         if (is_builtin) {
> -               printf("%-16s%s%c", "name:", kmod_module_get_name(mod), separator);
>                 filename = "(builtin)";
>         }
>
> -       if (field != NULL && streq(field, "filename")) {
> +       if (field != NULL && streq(field, "name")) {
> +               printf("%s%c", kmod_module_get_name(mod),
> +                              separator);
> +               return 0;
> +       } else if (field != NULL && streq(field, "filename")) {
>                 printf("%s%c", filename, separator);
>                 return 0;
>         } else if (field == NULL) {
> +               if (is_builtin) {
> +                       printf("%-16s%s%c", "name:",
> +                              kmod_module_get_name(mod), separator);
> +               }
>                 printf("%-16s%s%c", "filename:",
>                        filename, separator);
>         }
> --
> 2.28.0
>