Re: [PATCH] modinfo: handle arguments more carefully

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

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.


Regards,
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