Re: [PATCH] modinfo: handle arguments more carefully

[Lucas De Marchi <lucas.demarchi@xxxxxxxxxxxxxx>]

On Sat, Feb 4, 2012 at 11:36 AM, Lucas De Marchi
<lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> Hi Dan
>
> * Dan McGee <dan@xxxxxxxxxxxxx> [2012-02-03 20:25:00 -0600]:
>
>> A simple case of breakage before this commit:
>>
>>     $ touch aes
>>     $ modinfo aes
>>     filename:       /tmp/aes
>>     ERROR: could not get modinfo from 'aes': Invalid argument
>>
>
> We had a similar problem with modprobe and decided removing the path
> option. However for modinfo I think what you did is a good idea.
>
>> Add a new is_module_filename() function that attempts to do more than
>> just check if the passed argument is a regular file. We look at the name
>> for a '.ko' string, and if that is found, ensure it is either at the end
>> of the string or followed by another '.' (for .gz and .xz modules, for
>> instance). We don't make this second option conditional on the way the
>> tools are built with compression support; the file is a module file
>> regardless and should always be treated that way.
>>
>
> Yes... I just hope there will not be bug reports regardin this. Bare in
> mind module aliases may contain dots.
>
>
>> When doing this, and noticed in the test suite output, we open the
>> system modules index unconditionally, even if it is never going to be
>> used during the modinfo call, which is the case when passing module
>> filenames directly. Delay the opening of the index file until we get an
>> argument that is not a module filename.
>
> Go one step further and remove the call to kmod_load_resources().
> There's no point in calling it.
>
>>
>> With-help-from: Dave Reisner <dreisner@xxxxxxxxxxxxx>
>> Signed-off-by: Dan McGee <dan@xxxxxxxxxxxxx>
>> ---
>>  tools/kmod-modinfo.c |   28 ++++++++++++++++++++++++----
>>  1 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/kmod-modinfo.c b/tools/kmod-modinfo.c
>> index 87483a5..bad344c 100644
>> --- a/tools/kmod-modinfo.c
>> +++ b/tools/kmod-modinfo.c
>> @@ -19,6 +19,7 @@
>>
>>  #include <stdio.h>
>>  #include <stdlib.h>
>> +#include <stdbool.h>
>>  #include <getopt.h>
>>  #include <errno.h>
>>  #include <string.h>
>> @@ -332,6 +333,21 @@ static void help(const char *progname)
>>               progname);
>>  }
>>
>> +static bool is_module_filename(const char *name)
>> +{
>> +     struct stat st;
>> +     const char *ptr;
>> +     if (stat(name, &st) == 0 && S_ISREG(st.st_mode) &&
>> +                     (ptr = strstr(name, ".ko")) != NULL) {
>> +             /* we screened for .ko; make sure this is either at the end of the name
>> +              * or followed by another '.' (e.g. gz or xz modules) */
>
> 80 chars?
>
>> +             if(ptr[3] != '\0' && ptr[3] != '.')
>> +                     return false;
>> +             return true;
>> +     }
>> +     return false;
>> +}
>> +
>>  static int do_modinfo(int argc, char *argv[])
>>  {
>>       struct kmod_ctx *ctx;
>> @@ -341,6 +357,7 @@ static int do_modinfo(int argc, char *argv[])
>>       const char *root = NULL;
>>       const char *null_config = NULL;
>>       int i, err;
>> +     bool resources_loaded = false;
>>
>>       for (;;) {
>>               int c, idx = 0;
>> @@ -418,18 +435,21 @@ static int do_modinfo(int argc, char *argv[])
>>               fputs("Error: kmod_new() failed!\n", stderr);
>>               return EXIT_FAILURE;
>>       }
>> -     kmod_load_resources(ctx);
>>
>>       err = 0;
>>       for (i = optind; i < argc; i++) {
>>               const char *name = argv[i];
>> -             struct stat st;
>>               int r;
>>
>> -             if (stat(name, &st) == 0 && S_ISREG(st.st_mode))
>> +             if (is_module_filename(name)) {
>>                       r = modinfo_path_do(ctx, name);
>> -             else
>> +             } else {
>> +                     if (!resources_loaded) {
>> +                             kmod_load_resources(ctx);
>> +                             resources_loaded = true;
>> +                     }
>
> As said, no need to kmod_load_resources. The reason is that it's only 1
> module that might be using the index, so just let the fallback
> implementation setup the necessary things later.


Since I was in a hurry for next release, I amended the patch myself.


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html