Re: [PATCH] modprobe: add --resolve-alias option

[Thomas Bächler <thomas@xxxxxxxxxxxxx>]

Alan Jenkins schrieb:
> On 8/26/09, Thomas Bächler <thomas@xxxxxxxxxxxxx> wrote:
> > The --resolve-alias option prints all module names that match the
> > alias given on the commandline and exits. If no valid alias is specified,
> > it does nothing.
>
> Why?
>
> I.e. we already have "--verbose --dry-run" for humans, so I assume
> this is for scripting purposes.  Which scripts would you like to use
> this in?

- modprobe --show-depends

Will resolve dependencies, I only want the exact module names for the 
aliases.

- modprobe --verbose --dry-run

Will only show names for modules that are not already loaded. I want all 
names, regardless of whether they are loaded or not (now that I think 
about it, I want that for testing purposes, but in the real world it 
wouldn't matter). Like the above command, this one will resolve 
dependencies as well.

Both also show "insmod" and the full path, but you are right that some 
cut/basename magic will solve that. It still seems pointless to have 
modprobe resolve the path first and then have a script throw it away again.

I'm using this for an advanced blacklisting mechanism, that works like this:
- Resolve all module names for an alias
- For each of those module names:
  - Resolve dependencies
  - Match the module and all of its dependencies against a blacklist
  - If it doesn't match, load it

The commands mentioned above will provide me with a mix of dependencies 
for all module names, instead of a separate list of dependencies for 
each of them.

Besides this application, it would be really convenient in daily life to 
be able to just know "what the heck is this alias for?". It seems like 
an obvious feature to me.

> > @@ -1342,20 +1343,23 @@ int do_modprobe(char *modname,
> >  		if (aliases->next)
> >  			err = warn;
> >  		while (aliases) {
> > -			/* Add the options for this alias. */
> > -			char *opts = NOFAIL(strdup(cmdline_opts));
> > -			opts = add_extra_options(modname,
> > -						 opts, modoptions);
> > -
> > -			read_depends(dirname, aliases->module, &list);
> > -			failed |= handle_module(aliases->module,
> > -				&list, newname, opts, modoptions,
> > -				commands, cmdline_opts, err, flags);
> > -
> > +			if(flags & mit_resolve_alias) {
> > +				info("%s\n", aliases->module);
>
> I think printf() would be better.

I just tried to match whatever you used in other places in modprobe, if 
you say I should use printf, then I have no objections.

> > +			} else {
> > +				/* Add the options for this alias. */
> > +				char *opts = NOFAIL(strdup(cmdline_opts));
> > +				opts = add_extra_options(modname,
> > +							 opts, modoptions);
> > +
> > +				read_depends(dirname, aliases->module, &list);
> > +				failed |= handle_module(aliases->module,
> > +					&list, newname, opts, modoptions,
> > +					commands, cmdline_opts, err, flags);
> > +				INIT_LIST_HEAD(&list);
> > +			}
> >  			aliases = aliases->next;
> > -			INIT_LIST_HEAD(&list);
> >  		}
> > -	} else {
> > +	} else if(!(flags & mit_resolve_alias)) {
> >  		if (flags & mit_use_blacklist
> >  		    && find_blacklist(modname, blacklist))
> >  			return failed;
>
> Yay, more special cases and higher nesting levels :-).
>
> How about this instead?
>
>  	aliases = apply_blacklist(aliases, blacklist);
> +	if(flags & mit_resolve_alias) {
> +		while(aliases) {
> +			printf("%s\n", aliases->module);
> +			aliases = aliases->next;
> +		}
> +		return 0;
> +	}

Looks better, yes, also makes the diff cleaner.

> It could be even shorter if you don't mind switching the while() loops
> to for() loops as well.

Like this?

for(; aliases; aliases=aliases->next)
  printf("%s\n", aliases->module);
return 0;

> Regards
> Alan

Should I recreate the patch with the above changes, or are you going to 
reject it anyway?

Best regards
Thomas

Attachment: signature.asc
Description: OpenPGP digital signature