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

[Lucas De Marchi <lucas.de.marchi@gmail.com>]

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
>