On Wed, Aug 26, 2020 at 2:36 AM Xaver Hörl <hoe.dom@xxxxxx> 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
>
