linux-modules.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] modinfo: dont print module name when --field is given
@ 2020-08-23 21:54 Xaver Hörl
  2020-08-25 17:27 ` Lucas De Marchi
  0 siblings, 1 reply; 4+ messages in thread
From: Xaver Hörl @ 2020-08-23 21:54 UTC (permalink / raw)
  To: linux-modules

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] modinfo: dont print module name when --field is given
  2020-08-23 21:54 [PATCH] modinfo: dont print module name when --field is given Xaver Hörl
@ 2020-08-25 17:27 ` Lucas De Marchi
  2020-08-26  9:36   ` Xaver Hörl
  0 siblings, 1 reply; 4+ messages in thread
From: Lucas De Marchi @ 2020-08-25 17:27 UTC (permalink / raw)
  To: Xaver Hörl; +Cc: linux-modules

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
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] modinfo: dont print module name when --field is given
  2020-08-25 17:27 ` Lucas De Marchi
@ 2020-08-26  9:36   ` Xaver Hörl
  2020-08-26 10:12     ` Lucas De Marchi
  0 siblings, 1 reply; 4+ messages in thread
From: Xaver Hörl @ 2020-08-26  9:36 UTC (permalink / raw)
  To: linux-modules; +Cc: Lucas De Marchi

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] modinfo: dont print module name when --field is given
  2020-08-26  9:36   ` Xaver Hörl
@ 2020-08-26 10:12     ` Lucas De Marchi
  0 siblings, 0 replies; 4+ messages in thread
From: Lucas De Marchi @ 2020-08-26 10:12 UTC (permalink / raw)
  To: Xaver Hörl; +Cc: linux-modules

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
>

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-08-26 10:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-23 21:54 [PATCH] modinfo: dont print module name when --field is given Xaver Hörl
2020-08-25 17:27 ` Lucas De Marchi
2020-08-26  9:36   ` Xaver Hörl
2020-08-26 10:12     ` Lucas De Marchi

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox