[Toybox] [PATCH] Re: modinfo...

[Rob Landley]

On 06/20/2013 11:24:48 PM, Isaac wrote:
> On Wed, Jun 19, 2013 at 11:44:08PM -0500, Rob Landley wrote:
> > On 06/18/2013 08:55:42 AM, Isaac wrote:
> > >
> > >Oh, there's a trivial option to add to modinfo: -b <basedir>
> > >(used for constructing an initrd-it locates the modules within
> > >a given directory).
> >
> > Um, where's this from? My host's modinfo hasn't got it, and I don't
> > know of an actual standard for this...
> >
> 
> I noticed it on my Lucid system:

Ok.

> So I don't see a high priority; on the other hand, it might  
> hypothetically be
> useful if firmware requirements change in a new kernel version.
> In other words, sounds like something to hold off on.

I haven't got an objection if it's actually being used somewhere, I  
just didn't see it on my system.

> > >I have a patch to add it, but I'm wondering why
> > >NEWTOY(..."<b:F:0"...)
> > >GLOBALS(
> > >  char *field;
> > >  char *basedir;
> > >..
> > >works right, and putting the GLOBALS in the same order as the
> > >option string breaks (tested on glibc and musl).
> >
> > The option strings parse from right to left. F: is the rightmost
> > captured string so it goes in the first slot, b: is in the next
> > rightmost captured string, so it goes in the second slot.
> >
> > The documentation describes this:
> > http://landley.net/toybox/code.html#lib_args
> >
> > >modinfo also should support specifying an absolute or relative path
> > >to a module; the one complication is that modinfo_file() takes a
> > >struct dirtree, not a filename; can dirtree_read handle a filename
> > >or a relative path?
> >
> > Yes: http://landley.net/toybox/code.html#lib_dirtree
> 
> Thanks for the answer; I missed that, probably due to trying to read
> rather late.

I feel I should make the documentation more clear, but dunno how.

> I have a patch that assumes that the presence of the string ".ko"  
> indicates
> use of a path to a module (*.ko.xz and similar included, but not  
> supported).

Wouldn't the presence of a / character indicate use of a path?

> While I am not aware of any rules forbidding a module like  
> "barf.ko.ko",
> an strace of module-init-tools modinfo indicates that they use  
> similar logic.

Hmmm... I guess an actual file is going to have the .ko extension,  
although strstr() doesn't confirm it _ends_ with .ko...

The difference between if (strchr(str, '/')) and checking for .ko is  
that "modinfo filename.ko" would work with the second but not the  
first. We could also always check for the argument as a file, but that  
would mean if you do have a file in the current directory of the same  
name it would load that file, and that's a bit like not having "." in  
the $PATH by default...

Eh, changing behavior based on extension is normalish. I'd like to add  
a check to make sure it's actually at the end until the further  
extension support is implemented.

> It adds 6 lines, with 41 bytes increase in binary size (GCC 4.4,
> glibc/dynamic and musl/static, according to make baseline/bloatcheck).
> I'm not sure if the void->int change was needed, though it does keep  
> GCC
> quiet. But perror_exit; -> perror_msg; return; was needed.

Except it's still xopen(), so it'll abort if unable to open the file,  
but return error if it can't map it. (And leak the filehandle.) Hmmm...  
all modinfo_file() actually uses is the filename, no reason to go  
through dirtree() for that... Nothing is actually _checking_ the return  
value of modinfo_file()... Huh, and this thing is doing a blob of  
global data (I try to combine globals into the union)...

Checking in a cleanup on top of your commit. (And I need to dig up the  
-b patch...)

Thanks,

Rob