[PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny K N]

usecase: two sd cards are being mounted in parallel at same time on
dual core. example modules which are getting loaded is nls_cp437.
While one module is being loaded , it starts creating sysfs files.
meanwhile on other core, modprobe might return saying the module
is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.

Experiments done to prove the issue in kmod.
Added sleep in kernel module.c at the place of creation of sysfs files.
Then tried `modprobe nls_cp437` from two different shells.
While the first was still waiting for its completion ,
the second one returned saying the module is built-in.

built-in modules are handled by searching the modules.builtin file.
mod->builtin gets set and are handled in kmod_module_get_initstate function.
Removing the checking of the presence of /sys/module/<modulename>/
directory, which may not be required. It has to be added in other place
accordingly if required.

Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
 libkmod/libkmod-module.c |   13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 30f15ca..21c2a7e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -1708,7 +1708,7 @@ KMOD_EXPORT const char *kmod_module_initstate_str(enum kmod_module_initstate sta
 KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
 {
 	char path[PATH_MAX], buf[32];
-	int fd, err, pathlen;
+	int fd, err;
 
 	if (mod == NULL)
 		return -ENOENT;
@@ -1716,7 +1716,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
 	if (mod->builtin)
 		return KMOD_MODULE_BUILTIN;
 
-	pathlen = snprintf(path, sizeof(path),
+	snprintf(path, sizeof(path),
 				"/sys/module/%s/initstate", mod->name);
 	fd = open(path, O_RDONLY|O_CLOEXEC);
 	if (fd < 0) {
@@ -1725,15 +1725,6 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
 		DBG(mod->ctx, "could not open '%s': %s\n",
 			path, strerror(-err));
 
-		if (pathlen > (int)sizeof("/initstate") - 1) {
-			struct stat st;
-			path[pathlen - (sizeof("/initstate") - 1)] = '\0';
-			if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
-				return KMOD_MODULE_BUILTIN;
-		}
-
-		DBG(mod->ctx, "could not open '%s': %s\n",
-			path, strerror(-err));
 		return err;
 	}
 
-- 
1.7.9.5

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
> usecase: two sd cards are being mounted in parallel at same time on
> dual core. example modules which are getting loaded is nls_cp437.
> While one module is being loaded , it starts creating sysfs files.
> meanwhile on other core, modprobe might return saying the module
> is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.

an {f,}init_module() call should not return until the sysfs files are
created and if there is /sys/module/<module>/ there should be
/sys/module/<module>/initstate unless the module is builtin.

Rusty, was there any changes in this area in the kernel recently?

Or is the creation of /sys/module/<module> and
/sys/module/<module>/initstate not atomic?

See patch below.


-- 
Lucas De Marchi

>
> Experiments done to prove the issue in kmod.
> Added sleep in kernel module.c at the place of creation of sysfs files.
> Then tried `modprobe nls_cp437` from two different shells.
> While the first was still waiting for its completion ,
> the second one returned saying the module is built-in.
>
> built-in modules are handled by searching the modules.builtin file.
> mod->builtin gets set and are handled in kmod_module_get_initstate function.
> Removing the checking of the presence of /sys/module/<modulename>/
> directory, which may not be required. It has to be added in other place
> accordingly if required.
>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> ---
>  libkmod/libkmod-module.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 30f15ca..21c2a7e 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -1708,7 +1708,7 @@ KMOD_EXPORT const char *kmod_module_initstate_str(enum kmod_module_initstate sta
>  KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>  {
>         char path[PATH_MAX], buf[32];
> -       int fd, err, pathlen;
> +       int fd, err;
>
>         if (mod == NULL)
>                 return -ENOENT;
> @@ -1716,7 +1716,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>         if (mod->builtin)
>                 return KMOD_MODULE_BUILTIN;
>
> -       pathlen = snprintf(path, sizeof(path),
> +       snprintf(path, sizeof(path),
>                                 "/sys/module/%s/initstate", mod->name);
>         fd = open(path, O_RDONLY|O_CLOEXEC);
>         if (fd < 0) {
> @@ -1725,15 +1725,6 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>                 DBG(mod->ctx, "could not open '%s': %s\n",
>                         path, strerror(-err));
>
> -               if (pathlen > (int)sizeof("/initstate") - 1) {
> -                       struct stat st;
> -                       path[pathlen - (sizeof("/initstate") - 1)] = '\0';
> -                       if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
> -                               return KMOD_MODULE_BUILTIN;
> -               }
> -
> -               DBG(mod->ctx, "could not open '%s': %s\n",
> -                       path, strerror(-err));
>                 return err;
>         }
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-modules" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Rusty Russell]

Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
> On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>> usecase: two sd cards are being mounted in parallel at same time on
>> dual core. example modules which are getting loaded is nls_cp437.
>> While one module is being loaded , it starts creating sysfs files.
>> meanwhile on other core, modprobe might return saying the module
>> is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.
>
> an {f,}init_module() call should not return until the sysfs files are
> created and if there is /sys/module/<module>/ there should be
> /sys/module/<module>/initstate unless the module is builtin.
>
> Rusty, was there any changes in this area in the kernel recently?

Not deliberately.

> Or is the creation of /sys/module/<module> and
> /sys/module/<module>/initstate not atomic?

No, it's not atomic :(  That makes it unreliable (it seemed like a good
idea in 2006 I guess).

Here's what happens from a kernel side:

1) Module is marked UNFORMED.
2) Check there's no module by same name already.  If there is, and it's
   UNFORMED or COMING, we wait.
3) module is marked COMING
4) module parses arguments.
5) sysfs directory and attributes are created.
6) module's init is called.
7) module is marked LIVE.

So, the second init_module should be waiting.  

I tested this by putting a sleep in the nls_cp437 init, and watching
what modprobe did.  It works correctly.

You are checking initstate, and getting caught in the race:

783   14:33:14.170205 open("/sys/module/nls_cp437/initstate", O_RDONLY|O_LARGEFILE|O_CLOEXEC)

You should probably check initstate *after* loading fails.  It's
possible that it's unloaded in the meantime, but the race is pretty
narrow and unloading is unusual.

Cheers,
Rusty.

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Wednesday 18 February 2015 09:37 AM, Rusty Russell wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
>> <harish_kandiga@mentor.com> wrote:
>>> usecase: two sd cards are being mounted in parallel at same time on
>>> dual core. example modules which are getting loaded is nls_cp437.
>>> While one module is being loaded , it starts creating sysfs files.
>>> meanwhile on other core, modprobe might return saying the module
>>> is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.
>> an {f,}init_module() call should not return until the sysfs files are
>> created and if there is /sys/module/<module>/ there should be
>> /sys/module/<module>/initstate unless the module is builtin.
>>
>> Rusty, was there any changes in this area in the kernel recently?
> Not deliberately.
>
>> Or is the creation of /sys/module/<module> and
>> /sys/module/<module>/initstate not atomic?
> No, it's not atomic :(  That makes it unreliable (it seemed like a good
> idea in 2006 I guess).
>
> Here's what happens from a kernel side:
>
> 1) Module is marked UNFORMED.
> 2) Check there's no module by same name already.  If there is, and it's
>    UNFORMED or COMING, we wait.
> 3) module is marked COMING
> 4) module parses arguments.
> 5) sysfs directory and attributes are created.
> 6) module's init is called.
> 7) module is marked LIVE.
These are the operations handled in kernel after syscall/init_module is called. Here is the list of operations which happen before point number 1)
0a) __request_module/try_then_request_module gets called from kernel.
0b) call_modprobe gets called which calls usermode modprobe to see if module is loaded.
0c) syscall init_module gets called from modprobe.
> So, the second init_module should be waiting.  
>
> I tested this by putting a sleep in the nls_cp437 init, and watching
> what modprobe did.  It works correctly.
Problem here is second init_module is not yet called. if it gets called , it is handled properly. Adding a sleep in nls_cp437 is handled properly.
We need to add sleep in module_add_modinfo_attrs ( module.c ) while creating sysfs files(sysfs_create_file) in order to reproduce the issue I mentioned.
>
> You are checking initstate, and getting caught in the race:
>
> 783   14:33:14.170205 open("/sys/module/nls_cp437/initstate", O_RDONLY|O_LARGEFILE|O_CLOEXEC)
>
> You should probably check initstate *after* loading fails.  It's
> possible that it's unloaded in the meantime, but the race is pretty
> narrow and unloading is unusual.
Did not get the point of checking initstate after loading fails. However we need to handle even unusual rare cases as well.
>
> Cheers,
> Rusty.

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> On Tue, Feb 17, 2015 at 10:56 AM, Harish Jenny K N
>> <harish_kandiga@mentor.com> wrote:
>>> usecase: two sd cards are being mounted in parallel at same time on
>>> dual core. example modules which are getting loaded is nls_cp437.
>>> While one module is being loaded , it starts creating sysfs files.
>>> meanwhile on other core, modprobe might return saying the module
>>> is KMOD_MODULE_BUILTIN, which might result in not mounting sd card.
>>
>> an {f,}init_module() call should not return until the sysfs files are
>> created and if there is /sys/module/<module>/ there should be
>> /sys/module/<module>/initstate unless the module is builtin.
>>
>> Rusty, was there any changes in this area in the kernel recently?
>
> Not deliberately.
>
>> Or is the creation of /sys/module/<module> and
>> /sys/module/<module>/initstate not atomic?
>
> No, it's not atomic :(  That makes it unreliable (it seemed like a good
> idea in 2006 I guess).
>
> Here's what happens from a kernel side:
>
> 1) Module is marked UNFORMED.
> 2) Check there's no module by same name already.  If there is, and it's
>    UNFORMED or COMING, we wait.
> 3) module is marked COMING
> 4) module parses arguments.
> 5) sysfs directory and attributes are created.
> 6) module's init is called.
> 7) module is marked LIVE.

Yeah, I just thought (an wanted that) the attributes were being
created first and then hooked up in the sysfs tree under
/sys/module/<modulename>. I.e. if the directory exists and there's no
initstate this is because it's a builtin module. I don't want to
wait/sleep on the file to appear because users of
kmod_module_get_initstate() may not tolerate this behavior.

Looking up at the old module-init-tools, it used an ugly loop with
usleep() before trying to read the file again :-/

Can we change kernel side guaranteeing the initstate file appears
together with the directory?

>
> So, the second init_module should be waiting.
>
> I tested this by putting a sleep in the nls_cp437 init, and watching
> what modprobe did.  It works correctly.
>
> You are checking initstate, and getting caught in the race:
>
> 783   14:33:14.170205 open("/sys/module/nls_cp437/initstate", O_RDONLY|O_LARGEFILE|O_CLOEXEC)
>
> You should probably check initstate *after* loading fails.  It's
> possible that it's unloaded in the meantime, but the race is pretty
> narrow and unloading is unusual.

This call may be called in other paths, not while loading a module.
Otherwise just removing the check like what this patch does would be
sufficient.

We don't check the initstate after loading fails because we rely on
the return code of init_module, i.e. errno==EEXISTS if the previous
call succeeded.


-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Rusty Russell]

Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Yeah, I just thought (an wanted that) the attributes were being
> created first and then hooked up in the sysfs tree under
> /sys/module/<modulename>. I.e. if the directory exists and there's no
> initstate this is because it's a builtin module. I don't want to
> wait/sleep on the file to appear because users of
> kmod_module_get_initstate() may not tolerate this behavior.
>
> Looking up at the old module-init-tools, it used an ugly loop with
> usleep() before trying to read the file again :-/
>
> Can we change kernel side guaranteeing the initstate file appears
> together with the directory?

Greg?  The core problem is that kmod looks for
/sys/module/<name>/initstate; if it's not there, it assumes a builtin
module.

However, this is racy when a module is being inserted.  Is there a way
to create this sysfs file and dir atomically?

Thanks,
Rusty.

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Yeah, I just thought (an wanted that) the attributes were being
>> created first and then hooked up in the sysfs tree under
>> /sys/module/<modulename>. I.e. if the directory exists and there's no
>> initstate this is because it's a builtin module. I don't want to
>> wait/sleep on the file to appear because users of
>> kmod_module_get_initstate() may not tolerate this behavior.
>>
>> Looking up at the old module-init-tools, it used an ugly loop with
>> usleep() before trying to read the file again :-/
>>
>> Can we change kernel side guaranteeing the initstate file appears
>> together with the directory?
>
> Greg?  The core problem is that kmod looks for
> /sys/module/<name>/initstate; if it's not there, it assumes a builtin
> module.

Just to make it clear:

We try to open /sys/module/<name>/initstate. If it fails we stat
/sys/module/<name> checking if it exists and is a directory. If it
does then we assume the module is builtin.

> However, this is racy when a module is being inserted.  Is there a way
> to create this sysfs file and dir atomically?

Greg, the question is still valid since it'd be nice to have this
guarantee and be able to correctly reply the state with whatever is in
initstate file, but...

Rusty, thinking again if we fallback to "coming" instead of "builtin"
everything should be fine, no? Because the decision about builtin has
already been taken by looking at the modules.builtin index. If we
return "coming" here the second call to modprobe would call
init_module() again which would wait for the first one to complete (or
return EEXIST if it's already live) since we only shortcut the
init_module() call if the module is live or builtin

what do you think?


Harrish, in your patch if you just change the "return
KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?

-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[greg KH]

On Wed, Feb 18, 2015 at 11:19:14PM -0200, Lucas De Marchi wrote:
> On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
> >> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> >> Yeah, I just thought (an wanted that) the attributes were being
> >> created first and then hooked up in the sysfs tree under
> >> /sys/module/<modulename>. I.e. if the directory exists and there's no
> >> initstate this is because it's a builtin module. I don't want to
> >> wait/sleep on the file to appear because users of
> >> kmod_module_get_initstate() may not tolerate this behavior.
> >>
> >> Looking up at the old module-init-tools, it used an ugly loop with
> >> usleep() before trying to read the file again :-/
> >>
> >> Can we change kernel side guaranteeing the initstate file appears
> >> together with the directory?
> >
> > Greg?  The core problem is that kmod looks for
> > /sys/module/<name>/initstate; if it's not there, it assumes a builtin
> > module.
> 
> Just to make it clear:
> 
> We try to open /sys/module/<name>/initstate. If it fails we stat
> /sys/module/<name> checking if it exists and is a directory. If it
> does then we assume the module is builtin.
> 
> > However, this is racy when a module is being inserted.  Is there a way
> > to create this sysfs file and dir atomically?
> 
> Greg, the question is still valid since it'd be nice to have this
> guarantee and be able to correctly reply the state with whatever is in
> initstate file, but...

You should always wait until you get the uevent that the object was
added before poking around in sysfs.  The kernel will guarantee all of
the needed files / directories will be created before that event is sent
out.  That's why we added the uevent message.

So by just busy-spinning on the directory and ignoring the uevent, you
are just blindly guessing as to when things are finished, which is racy
as you see.

So please just wait for the event, then you should be fine, that's what
it is there for.

thanks,

greg k-h

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Rusty Russell]

Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
> On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>>> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Yeah, I just thought (an wanted that) the attributes were being
>>> created first and then hooked up in the sysfs tree under
>>> /sys/module/<modulename>. I.e. if the directory exists and there's no
>>> initstate this is because it's a builtin module. I don't want to
>>> wait/sleep on the file to appear because users of
>>> kmod_module_get_initstate() may not tolerate this behavior.
>>>
>>> Looking up at the old module-init-tools, it used an ugly loop with
>>> usleep() before trying to read the file again :-/
>>>
>>> Can we change kernel side guaranteeing the initstate file appears
>>> together with the directory?
>>
>> Greg?  The core problem is that kmod looks for
>> /sys/module/<name>/initstate; if it's not there, it assumes a builtin
>> module.
>
> Just to make it clear:
>
> We try to open /sys/module/<name>/initstate. If it fails we stat
> /sys/module/<name> checking if it exists and is a directory. If it
> does then we assume the module is builtin.
>
>> However, this is racy when a module is being inserted.  Is there a way
>> to create this sysfs file and dir atomically?
>
> Greg, the question is still valid since it'd be nice to have this
> guarantee and be able to correctly reply the state with whatever is in
> initstate file, but...
>
> Rusty, thinking again if we fallback to "coming" instead of "builtin"
> everything should be fine, no? Because the decision about builtin has
> already been taken by looking at the modules.builtin index. If we
> return "coming" here the second call to modprobe would call
> init_module() again which would wait for the first one to complete (or
> return EEXIST if it's already live) since we only shortcut the
> init_module() call if the module is live or builtin

It's weird that your code should care about this at all.  Ideally,
userspace would see builtin modules as simply unremovable ones.
Historically, it hasn't; it was only module parameters for builtins
which caused us to expose built modules.

If we returned EEXIST for builtin modules, would you have to do the
initstate check at all?

Thanks,
Rusty.

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 12:25 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>>>> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>>> Yeah, I just thought (an wanted that) the attributes were being
>>>> created first and then hooked up in the sysfs tree under
>>>> /sys/module/<modulename>. I.e. if the directory exists and there's no
>>>> initstate this is because it's a builtin module. I don't want to
>>>> wait/sleep on the file to appear because users of
>>>> kmod_module_get_initstate() may not tolerate this behavior.
>>>>
>>>> Looking up at the old module-init-tools, it used an ugly loop with
>>>> usleep() before trying to read the file again :-/
>>>>
>>>> Can we change kernel side guaranteeing the initstate file appears
>>>> together with the directory?
>>>
>>> Greg?  The core problem is that kmod looks for
>>> /sys/module/<name>/initstate; if it's not there, it assumes a builtin
>>> module.
>>
>> Just to make it clear:
>>
>> We try to open /sys/module/<name>/initstate. If it fails we stat
>> /sys/module/<name> checking if it exists and is a directory. If it
>> does then we assume the module is builtin.
>>
>>> However, this is racy when a module is being inserted.  Is there a way
>>> to create this sysfs file and dir atomically?
>>
>> Greg, the question is still valid since it'd be nice to have this
>> guarantee and be able to correctly reply the state with whatever is in
>> initstate file, but...
>>
>> Rusty, thinking again if we fallback to "coming" instead of "builtin"
>> everything should be fine, no? Because the decision about builtin has
>> already been taken by looking at the modules.builtin index. If we
>> return "coming" here the second call to modprobe would call
>> init_module() again which would wait for the first one to complete (or
>> return EEXIST if it's already live) since we only shortcut the
>> init_module() call if the module is live or builtin
>
> It's weird that your code should care about this at all.  Ideally,
> userspace would see builtin modules as simply unremovable ones.
> Historically, it hasn't; it was only module parameters for builtins
> which caused us to expose built modules.

yeah... ideally all modules would have their entries in /sys/module

> If we returned EEXIST for builtin modules, would you have to do the
> initstate check at all?

In this case there would be some behavior changes wrt blacklist,
softdeps and builtin modules. Currently if the module is live/builtin
we just return success (oh, unless --first-time is passed :-/).
Otherwise we apply blacklists, softdeps and dependencies. With this we
could reach a scenario in which we fail to load a builtin module which
is very weird.

And... the race for "lsmod" would still exist. Instead of thinking
about a race between 2 modprobe calls, think about a race between 1
modprobe and 1 lsmod. What do I answer for the module that is loading?
IMO the "coming" alternative is the one that makes sense (and would
also fix the 2 modprobes)

-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 12:25 AM, greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Feb 18, 2015 at 11:19:14PM -0200, Lucas De Marchi wrote:
>> On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> > Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>> >> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> >> Yeah, I just thought (an wanted that) the attributes were being
>> >> created first and then hooked up in the sysfs tree under
>> >> /sys/module/<modulename>. I.e. if the directory exists and there's no
>> >> initstate this is because it's a builtin module. I don't want to
>> >> wait/sleep on the file to appear because users of
>> >> kmod_module_get_initstate() may not tolerate this behavior.
>> >>
>> >> Looking up at the old module-init-tools, it used an ugly loop with
>> >> usleep() before trying to read the file again :-/
>> >>
>> >> Can we change kernel side guaranteeing the initstate file appears
>> >> together with the directory?
>> >
>> > Greg?  The core problem is that kmod looks for
>> > /sys/module/<name>/initstate; if it's not there, it assumes a builtin
>> > module.
>>
>> Just to make it clear:
>>
>> We try to open /sys/module/<name>/initstate. If it fails we stat
>> /sys/module/<name> checking if it exists and is a directory. If it
>> does then we assume the module is builtin.
>>
>> > However, this is racy when a module is being inserted.  Is there a way
>> > to create this sysfs file and dir atomically?
>>
>> Greg, the question is still valid since it'd be nice to have this
>> guarantee and be able to correctly reply the state with whatever is in
>> initstate file, but...
>
> You should always wait until you get the uevent that the object was
> added before poking around in sysfs.  The kernel will guarantee all of
> the needed files / directories will be created before that event is sent
> out.  That's why we added the uevent message.

for kmod I think I prefer the alternative of not needing it at all...
for daemons and other tools it makes sense indeed.

> So by just busy-spinning on the directory and ignoring the uevent, you
> are just blindly guessing as to when things are finished, which is racy
> as you see.

note there's no busy-spinning in kmod.... this was in
module-init-tools and it's what I'm saying I don't want to do. Not
something you want in a library.

-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Thursday 19 February 2015 06:49 AM, Lucas De Marchi wrote:
> On Wed, Feb 18, 2015 at 8:40 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>>> On Wed, Feb 18, 2015 at 2:07 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Yeah, I just thought (an wanted that) the attributes were being
>>> created first and then hooked up in the sysfs tree under
>>> /sys/module/<modulename>. I.e. if the directory exists and there's no
>>> initstate this is because it's a builtin module. I don't want to
>>> wait/sleep on the file to appear because users of
>>> kmod_module_get_initstate() may not tolerate this behavior.
>>>
>>> Looking up at the old module-init-tools, it used an ugly loop with
>>> usleep() before trying to read the file again :-/
>>>
>>> Can we change kernel side guaranteeing the initstate file appears
>>> together with the directory?
>> Greg?  The core problem is that kmod looks for
>> /sys/module/<name>/initstate; if it's not there, it assumes a builtin
>> module.
> Just to make it clear:
>
> We try to open /sys/module/<name>/initstate. If it fails we stat
> /sys/module/<name> checking if it exists and is a directory. If it
> does then we assume the module is builtin.
>
>> However, this is racy when a module is being inserted.  Is there a way
>> to create this sysfs file and dir atomically?
> Greg, the question is still valid since it'd be nice to have this
> guarantee and be able to correctly reply the state with whatever is in
> initstate file, but...
>
> Rusty, thinking again if we fallback to "coming" instead of "builtin"
> everything should be fine, no? Because the decision about builtin has
> already been taken by looking at the modules.builtin index. If we
> return "coming" here the second call to modprobe would call
> init_module() again which would wait for the first one to complete (or
> return EEXIST if it's already live) since we only shortcut the
> init_module() call if the module is live or builtin
>
> what do you think?
>
>
> Harrish, in your patch if you just change the "return
> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>


Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.

Do I need to send a separate patch ?

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
<harish_kandiga@mentor.com> wrote:
>> Harrish, in your patch if you just change the "return
>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>
> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. =
The built-in modules are handled by looking at the modules.builtin index fi=
le. Is there any chance of returning KMOD_MODULE_COMING for builti-in modul=
es? If it does not have any impact, then the fix should be fine.

well... you're not returning KMOD_MODULE_COMING for a builtin module.
Having the directory /sys/module/<name> and not the initstate could be
either that the module is builtin or that there's a race while loading
the module and it's in the coming state. However since we use the
index to decide if this module is builtin in the beginning of this
function, here it can only be the second case.

However... mod->builtin in the beginning of this function is only set
if the module is created by a lookup rather than from name or from
path.... maybe here we need to actually fallback to the index rather
than the cached value, otherwise this test would fail (considering
"vt" is builtin):

kmod_module_new_from_name(ctx, "vt", &mod);
kmod_module_get_initstate(mod, &state);


> Do I need to send a separate patch ?

I was hoping it would be a oneliner, but it isn't. If you are going to
send a patch, please add the necessary checks for the builtin index.
Otherwise I can take a look on this until the end of this week.

--=20
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
<harish_kandiga@mentor.com> wrote:
>> Harrish, in your patch if you just change the "return
>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>
> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.

well... you're not returning KMOD_MODULE_COMING for a builtin module.
Having the directory /sys/module/<name> and not the initstate could be
either that the module is builtin or that there's a race while loading
the module and it's in the coming state. However since we use the
index to decide if this module is builtin in the beginning of this
function, here it can only be the second case.

However... mod->builtin in the beginning of this function is only set
if the module is created by a lookup rather than from name or from
path.... maybe here we need to actually fallback to the index rather
than the cached value, otherwise this test would fail (considering
"vt" is builtin):

kmod_module_new_from_name(ctx, "vt", &mod);
kmod_module_get_initstate(mod, &state);


> Do I need to send a separate patch ?

I was hoping it would be a oneliner, but it isn't. If you are going to
send a patch, please add the necessary checks for the builtin index.
Otherwise I can take a look on this until the end of this week.

-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
> <harish_kandiga@mentor.com> wrote:
>>> Harrish, in your patch if you just change the "return
>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.
> well... you're not returning KMOD_MODULE_COMING for a builtin module.
> Having the directory /sys/module/<name> and not the initstate could be
> either that the module is builtin or that there's a race while loading
> the module and it's in the coming state. However since we use the
> index to decide if this module is builtin in the beginning of this
> function, here it can only be the second case.
>
> However... mod->builtin in the beginning of this function is only set
> if the module is created by a lookup rather than from name or from
> path.... maybe here we need to actually fallback to the index rather
> than the cached value, otherwise this test would fail (considering
> "vt" is builtin):
>
> kmod_module_new_from_name(ctx, "vt", &mod);
> kmod_module_get_initstate(mod, &state);
>



something like this ?

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
         * "module", except knowing it's builtin.
         */
        bool builtin : 1;
+
+       bool lookup : 1;
 };
 
 static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
        mod->builtin = builtin;
 }
 
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+       mod->lookup = lookup;
+}
+
 void kmod_module_set_required(struct kmod_module *mod, bool required)
 {
        mod->required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
                        struct stat st;
                        path[pathlen - (sizeof("/initstate") - 1)] = '\0';
                        if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
-                               return KMOD_MODULE_COMING;
+                               return mod->lookup ? KMOD_MODULE_COMING : KMOD_MODULE_BUILTIN;
                }
 
                DBG(mod->ctx, "could not open '%s': %s\n",
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3af16c7..43288b6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -549,6 +549,7 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
                if (lookup_only)
                        printf("%s\n", kmod_module_get_name(mod));
                else {
+                       kmod_module_set_lookup(mod,true);
                        err = kmod_module_probe_insert_module(mod, flags,
                                        extra_options, NULL, NULL, show);
                }



>> Do I need to send a separate patch ?
> I was hoping it would be a oneliner, but it isn't. If you are going to
> send a patch, please add the necessary checks for the builtin index.
> Otherwise I can take a look on this until the end of this week.
>

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
> <harish_kandiga@mentor.com> wrote:
>>> Harrish, in your patch if you just change the "return
>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.
> well... you're not returning KMOD_MODULE_COMING for a builtin module.
> Having the directory /sys/module/<name> and not the initstate could be
> either that the module is builtin or that there's a race while loading
> the module and it's in the coming state. However since we use the
> index to decide if this module is builtin in the beginning of this
> function, here it can only be the second case.
>
> However... mod->builtin in the beginning of this function is only set
> if the module is created by a lookup rather than from name or from
> path.... maybe here we need to actually fallback to the index rather
> than the cached value, otherwise this test would fail (considering
> "vt" is builtin):
>
> kmod_module_new_from_name(ctx, "vt", &mod);
> kmod_module_get_initstate(mod, &state);
>


something like this ?

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..d424f3e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -99,6 +99,8 @@ struct kmod_module {
         * "module", except knowing it's builtin.
         */
        bool builtin : 1;
+
+       bool lookup : 1;
 };
 
 static inline const char *path_join(const char *path, size_t prefixlen,
@@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
        mod->builtin = builtin;
 }
 
+void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
+{
+       mod->lookup = lookup;
+}
+
 void kmod_module_set_required(struct kmod_module *mod, bool required)
 {
        mod->required = required;
@@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
                        struct stat st;
                        path[pathlen - (sizeof("/initstate") - 1)] = '\0';
                        if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
-                               return KMOD_MODULE_COMING;
+                               return mod->lookup ? KMOD_MODULE_COMING : KMOD_MODULE_BUILTIN;
                }
 
                DBG(mod->ctx, "could not open '%s': %s\n",
diff --git a/tools/modprobe.c b/tools/modprobe.c
index 3af16c7..43288b6 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -549,6 +549,7 @@ static int insmod(struct kmod_ctx *ctx, const char *alias,
                if (lookup_only)
                        printf("%s\n", kmod_module_get_name(mod));
                else {
+                       kmod_module_set_lookup(mod,true);
                        err = kmod_module_probe_insert_module(mod, flags,
                                        extra_options, NULL, NULL, show);
                }





>> Do I need to send a separate patch ?
> I was hoping it would be a oneliner, but it isn't. If you are going to
> send a patch, please add the necessary checks for the builtin index.
> Otherwise I can take a look on this until the end of this week.
>

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
<harish_kandiga@mentor.com> wrote:
>
> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>> <harish_kandiga@mentor.com> wrote:
>>>> Harrish, in your patch if you just change the "return
>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works=
. The built-in modules are handled by looking at the modules.builtin index =
file. Is there any chance of returning KMOD_MODULE_COMING for builti-in mod=
ules? If it does not have any impact, then the fix should be fine.
>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>> Having the directory /sys/module/<name> and not the initstate could be
>> either that the module is builtin or that there's a race while loading
>> the module and it's in the coming state. However since we use the
>> index to decide if this module is builtin in the beginning of this
>> function, here it can only be the second case.
>>
>> However... mod->builtin in the beginning of this function is only set
>> if the module is created by a lookup rather than from name or from
>> path.... maybe here we need to actually fallback to the index rather
>> than the cached value, otherwise this test would fail (considering
>> "vt" is builtin):
>>
>> kmod_module_new_from_name(ctx, "vt", &mod);
>> kmod_module_get_initstate(mod, &state);
>>
>
>
>
> something like this ?
>
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 19bb2ed..d424f3e 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -99,6 +99,8 @@ struct kmod_module {
>          * "module", except knowing it's builtin.
>          */
>         bool builtin : 1;
> +
> +       bool lookup : 1;
>  };
>
>  static inline const char *path_join(const char *path, size_t prefixlen,
> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod=
, bool builtin)
>         mod->builtin =3D builtin;
>  }
>
> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
> +{
> +       mod->lookup =3D lookup;
> +}
> +
>  void kmod_module_set_required(struct kmod_module *mod, bool required)
>  {
>         mod->required =3D required;
> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const str=
uct kmod_module *mod)
>                         struct stat st;
>                         path[pathlen - (sizeof("/initstate") - 1)] =3D '\=
0';
>                         if (stat(path, &st) =3D=3D 0 && S_ISDIR(st.st_mod=
e))
> -                               return KMOD_MODULE_COMING;
> +                               return mod->lookup ? KMOD_MODULE_COMING :=
 KMOD_MODULE_BUILTIN;

no. I guess this doesn't pass the proposed test:

1)
kmod_module_new_from_name(ctx, "vt", &mod);
kmod_module_get_initstate(mod, &state);

this must return builtin

2)
kmod_module_new_from_lookup(ctx, "vt", &list);
... (get mod from list)
kmod_module_get_initstate(mod, &state);

this must return builtin as well.

I suggest you add a kmod_module_is_builtin() which does the lookup
(but doesn't increase the module refcount, i.e. doesn't call
new_module_xxxx()) iff it's not already done. For this you will need
to change mod->builtin to an enum: enum { XXXX_UNKNOWN, XXXX_NO,
XXXX_YES } then you do:

bool kmod_module_is_builtin(mod)  (don't export this function)
{
        if (mod->XXXX_UNKNOWN) {
              ... lookup in builtin index
        }
        return mod->builtin =3D=3D XXXX_YES;
}

then you change the users of mod->builtin.

--=20
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
<harish_kandiga@mentor.com> wrote:
>
> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>> <harish_kandiga@mentor.com> wrote:
>>>> Harrish, in your patch if you just change the "return
>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.
>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>> Having the directory /sys/module/<name> and not the initstate could be
>> either that the module is builtin or that there's a race while loading
>> the module and it's in the coming state. However since we use the
>> index to decide if this module is builtin in the beginning of this
>> function, here it can only be the second case.
>>
>> However... mod->builtin in the beginning of this function is only set
>> if the module is created by a lookup rather than from name or from
>> path.... maybe here we need to actually fallback to the index rather
>> than the cached value, otherwise this test would fail (considering
>> "vt" is builtin):
>>
>> kmod_module_new_from_name(ctx, "vt", &mod);
>> kmod_module_get_initstate(mod, &state);
>>
>
>
>
> something like this ?
>
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 19bb2ed..d424f3e 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -99,6 +99,8 @@ struct kmod_module {
>          * "module", except knowing it's builtin.
>          */
>         bool builtin : 1;
> +
> +       bool lookup : 1;
>  };
>
>  static inline const char *path_join(const char *path, size_t prefixlen,
> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>         mod->builtin = builtin;
>  }
>
> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
> +{
> +       mod->lookup = lookup;
> +}
> +
>  void kmod_module_set_required(struct kmod_module *mod, bool required)
>  {
>         mod->required = required;
> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>                         struct stat st;
>                         path[pathlen - (sizeof("/initstate") - 1)] = '\0';
>                         if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
> -                               return KMOD_MODULE_COMING;
> +                               return mod->lookup ? KMOD_MODULE_COMING : KMOD_MODULE_BUILTIN;

no. I guess this doesn't pass the proposed test:

1)
kmod_module_new_from_name(ctx, "vt", &mod);
kmod_module_get_initstate(mod, &state);

this must return builtin

2)
kmod_module_new_from_lookup(ctx, "vt", &list);
... (get mod from list)
kmod_module_get_initstate(mod, &state);

this must return builtin as well.

I suggest you add a kmod_module_is_builtin() which does the lookup
(but doesn't increase the module refcount, i.e. doesn't call
new_module_xxxx()) iff it's not already done. For this you will need
to change mod->builtin to an enum: enum { XXXX_UNKNOWN, XXXX_NO,
XXXX_YES } then you do:

bool kmod_module_is_builtin(mod)  (don't export this function)
{
        if (mod->XXXX_UNKNOWN) {
              ... lookup in builtin index
        }
        return mod->builtin == XXXX_YES;
}

then you change the users of mod->builtin.

-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
> <harish_kandiga@mentor.com> wrote:
>> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>>> <harish_kandiga@mentor.com> wrote:
>>>>> Harrish, in your patch if you just change the "return
>>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.
>>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>>> Having the directory /sys/module/<name> and not the initstate could be
>>> either that the module is builtin or that there's a race while loading
>>> the module and it's in the coming state. However since we use the
>>> index to decide if this module is builtin in the beginning of this
>>> function, here it can only be the second case.
>>>
>>> However... mod->builtin in the beginning of this function is only set
>>> if the module is created by a lookup rather than from name or from
>>> path.... maybe here we need to actually fallback to the index rather
>>> than the cached value, otherwise this test would fail (considering
>>> "vt" is builtin):
>>>
>>> kmod_module_new_from_name(ctx, "vt", &mod);
>>> kmod_module_get_initstate(mod, &state);
>>>
>>
>>
>> something like this ?
>>
>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index 19bb2ed..d424f3e 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -99,6 +99,8 @@ struct kmod_module {
>>          * "module", except knowing it's builtin.
>>          */
>>         bool builtin : 1;
>> +
>> +       bool lookup : 1;
>>  };
>>
>>  static inline const char *path_join(const char *path, size_t prefixlen,
>> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>>         mod->builtin = builtin;
>>  }
>>
>> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
>> +{
>> +       mod->lookup = lookup;
>> +}
>> +
>>  void kmod_module_set_required(struct kmod_module *mod, bool required)
>>  {
>>         mod->required = required;
>> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>>                         struct stat st;
>>                         path[pathlen - (sizeof("/initstate") - 1)] = '\0';
>>                         if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
>> -                               return KMOD_MODULE_COMING;
>> +                               return mod->lookup ? KMOD_MODULE_COMING : KMOD_MODULE_BUILTIN;
> no. I guess this doesn't pass the proposed test:
>
> 1)
> kmod_module_new_from_name(ctx, "vt", &mod);
> kmod_module_get_initstate(mod, &state);
>
> this must return builtin
>
> 2)
> kmod_module_new_from_lookup(ctx, "vt", &list);
> ... (get mod from list)
> kmod_module_get_initstate(mod, &state);
>
> this must return builtin as well.
>
> I suggest you add a kmod_module_is_builtin() which does the lookup
> (but doesn't increase the module refcount, i.e. doesn't call
> new_module_xxxx()) iff it's not already done. For this you will need
> to change mod->builtin to an enum: enum { XXXX_UNKNOWN, XXXX_NO,
> XXXX_YES } then you do:
>
> bool kmod_module_is_builtin(mod)  (don't export this function)
> {
>         if (mod->XXXX_UNKNOWN) {
>               ... lookup in builtin index
>         }
>         return mod->builtin == XXXX_YES;
> }
>
> then you change the users of mod->builtin.
>


something like this ?


diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 417f232..f6ffd3e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
 #  endif
 #endif
 
+/* Flags to kmod_builtin_status() */
+enum kmod_builtin_status {
+    KMOD_BUILTIN_UNKNOWN = 0,
+    KMOD_BUILTIN_NO = 1,
+    KMOD_BUILTIN_YES = 2
+};
+
 void kmod_log(const struct kmod_ctx *ctx,
         int priority, const char *file, int line, const char *fn,
         const char *format, ...) __attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));
@@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx *ctx, const char *name,
 int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
 int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
 int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name)__attribute__((nonnull(1, 2)));
 int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
 void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited) __attribute__((nonnull((1))));
 void kmod_set_modules_required(struct kmod_ctx *ctx, bool required) __attribute__((nonnull((1))));
@@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod, char *line) __attribute__
 void kmod_module_set_install_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
 void kmod_module_set_remove_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
 void kmod_module_set_visited(struct kmod_module *mod, bool visited) __attribute__((nonnull(1)));
-void kmod_module_set_builtin(struct kmod_module *mod, bool builtin) __attribute__((nonnull((1))));
+void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin) __attribute__((nonnull((1))));
 void kmod_module_set_required(struct kmod_module *mod, bool required) __attribute__((nonnull(1)));
-
+bool kmod_module_is_builtin(const struct kmod_module *mod);
 
 /* libkmod-file.c */
 struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filename) _must_check_ __attribute__((nonnull(1,2)));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..6b2f2f1 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -98,7 +98,7 @@ struct kmod_module {
      * is set. There's nothing much useful one can do with such a
      * "module", except knowing it's builtin.
      */
-    bool builtin : 1;
+    enum kmod_builtin_status builtin;
 };
 
 static inline const char *path_join(const char *path, size_t prefixlen,
@@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod, bool visited)
     mod->visited = visited;
 }
 
-void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
+void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin)
 {
     mod->builtin = builtin;
 }
@@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod, bool required)
     mod->required = required;
 }
 
+bool kmod_module_is_builtin(const struct kmod_module *mod)
+{
+    int builtin = mod->builtin;
+
+    if (builtin == KMOD_BUILTIN_UNKNOWN)
+        builtin = kmod_just_lookup_alias_from_builtin_file(mod->ctx, mod->name);
+
+    return mod->builtin == KMOD_BUILTIN_YES;
+}
 /*
  * Memory layout with alias:
  *
@@ -924,7 +933,7 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
                 module_is_blacklisted(mod))
             continue;
 
-        if ((filter_type & KMOD_FILTER_BUILTIN) && mod->builtin)
+        if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
             continue;
 
         node = kmod_list_append(*output, mod);
@@ -1713,7 +1722,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
     if (mod == NULL)
         return -ENOENT;
 
-    if (mod->builtin)
+    if (kmod_module_is_builtin(mod))
         return KMOD_MODULE_BUILTIN;
 
     pathlen = snprintf(path, sizeof(path),
diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 1a5a66b..d79bb12 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -525,7 +525,7 @@ int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name,
             goto finish;
         }
 
-        kmod_module_set_builtin(mod, true);
+        kmod_module_set_builtin(mod, KMOD_BUILTIN_YES);
         *list = kmod_list_append(*list, mod);
         if (*list == NULL)
             err = -ENOMEM;
@@ -536,6 +536,39 @@ finish:
     return err;
 }
 
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx,
+        const char *name) {
+    char *line = NULL;
+
+    if (ctx->indexes[KMOD_INDEX_MODULES_BUILTIN]) {
+        DBG(ctx, "use mmaped index '%s' modname=%s\n",
+                index_files[KMOD_INDEX_MODULES_BUILTIN].fn, name);
+        line = index_mm_search(ctx->indexes[KMOD_INDEX_MODULES_BUILTIN], name);
+    } else {
+        struct index_file *idx;
+        char fn[PATH_MAX];
+
+        snprintf(fn, sizeof(fn), "%s/%s.bin", ctx->dirname,
+                index_files[KMOD_INDEX_MODULES_BUILTIN].fn);
+        DBG(ctx, "file=%s modname=%s\n", fn, name);
+
+        idx = index_file_open(fn);
+        if (idx == NULL) {
+            DBG(ctx, "could not open builtin file '%s'\n", fn);
+            goto finish;
+        }
+
+        line = index_search(idx, name);
+        index_file_close(idx);
+    }
+
+    if (line != NULL)
+        return KMOD_BUILTIN_YES;
+
+finish:
+        return KMOD_BUILTIN_NO;
+}
+
 char *kmod_search_moddep(struct kmod_ctx *ctx, const char *name)
 {
     struct index_file *idx;

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
> On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
>> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
>> <harish_kandiga@mentor.com> wrote:
>>> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>>>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>>>> <harish_kandiga@mentor.com> wrote:
>>>>>> Harrish, in your patch if you just change the "return
>>>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN  works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.
>>>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>>>> Having the directory /sys/module/<name> and not the initstate could be
>>>> either that the module is builtin or that there's a race while loading
>>>> the module and it's in the coming state. However since we use the
>>>> index to decide if this module is builtin in the beginning of this
>>>> function, here it can only be the second case.
>>>>
>>>> However... mod->builtin in the beginning of this function is only set
>>>> if the module is created by a lookup rather than from name or from
>>>> path.... maybe here we need to actually fallback to the index rather
>>>> than the cached value, otherwise this test would fail (considering
>>>> "vt" is builtin):
>>>>
>>>> kmod_module_new_from_name(ctx, "vt", &mod);
>>>> kmod_module_get_initstate(mod, &state);
>>>>
>>>
>>> something like this ?
>>>
>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>> index 19bb2ed..d424f3e 100644
>>> --- a/libkmod/libkmod-module.c
>>> +++ b/libkmod/libkmod-module.c
>>> @@ -99,6 +99,8 @@ struct kmod_module {
>>>          * "module", except knowing it's builtin.
>>>          */
>>>         bool builtin : 1;
>>> +
>>> +       bool lookup : 1;
>>>  };
>>>
>>>  static inline const char *path_join(const char *path, size_t prefixlen,
>>> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>>>         mod->builtin = builtin;
>>>  }
>>>
>>> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
>>> +{
>>> +       mod->lookup = lookup;
>>> +}
>>> +
>>>  void kmod_module_set_required(struct kmod_module *mod, bool required)
>>>  {
>>>         mod->required = required;
>>> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>>>                         struct stat st;
>>>                         path[pathlen - (sizeof("/initstate") - 1)] = '\0';
>>>                         if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
>>> -                               return KMOD_MODULE_COMING;
>>> +                               return mod->lookup ? KMOD_MODULE_COMING : KMOD_MODULE_BUILTIN;
>> no. I guess this doesn't pass the proposed test:
>>
>> 1)
>> kmod_module_new_from_name(ctx, "vt", &mod);
>> kmod_module_get_initstate(mod, &state);
>>
>> this must return builtin
>>
>> 2)
>> kmod_module_new_from_lookup(ctx, "vt", &list);
>> ... (get mod from list)
>> kmod_module_get_initstate(mod, &state);
>>
>> this must return builtin as well.
>>
>> I suggest you add a kmod_module_is_builtin() which does the lookup
>> (but doesn't increase the module refcount, i.e. doesn't call
>> new_module_xxxx()) iff it's not already done. For this you will need
>> to change mod->builtin to an enum: enum { XXXX_UNKNOWN, XXXX_NO,
>> XXXX_YES } then you do:
>>
>> bool kmod_module_is_builtin(mod)  (don't export this function)
>> {
>>         if (mod->XXXX_UNKNOWN) {
>>               ... lookup in builtin index
>>         }
>>         return mod->builtin == XXXX_YES;
>> }
>>
>> then you change the users of mod->builtin.
>>
>
> something like this ?
>
>
> diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
> index 417f232..f6ffd3e 100644
> --- a/libkmod/libkmod-internal.h
> +++ b/libkmod/libkmod-internal.h
> @@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
>  #  endif
>  #endif
>  
> +/* Flags to kmod_builtin_status() */
> +enum kmod_builtin_status {
> +    KMOD_BUILTIN_UNKNOWN = 0,
> +    KMOD_BUILTIN_NO = 1,
> +    KMOD_BUILTIN_YES = 2
> +};
> +
>  void kmod_log(const struct kmod_ctx *ctx,
>          int priority, const char *file, int line, const char *fn,
>          const char *format, ...) __attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));
> @@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx *ctx, const char *name,
>  int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>  int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>  int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name)__attribute__((nonnull(1, 2)));
>  int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>  void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited) __attribute__((nonnull((1))));
>  void kmod_set_modules_required(struct kmod_ctx *ctx, bool required) __attribute__((nonnull((1))));
> @@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod, char *line) __attribute__
>  void kmod_module_set_install_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
>  void kmod_module_set_remove_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
>  void kmod_module_set_visited(struct kmod_module *mod, bool visited) __attribute__((nonnull(1)));
> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin) __attribute__((nonnull((1))));
> +void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin) __attribute__((nonnull((1))));
>  void kmod_module_set_required(struct kmod_module *mod, bool required) __attribute__((nonnull(1)));
> -
> +bool kmod_module_is_builtin(const struct kmod_module *mod);
>  
>  /* libkmod-file.c */
>  struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filename) _must_check_ __attribute__((nonnull(1,2)));
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index 19bb2ed..6b2f2f1 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -98,7 +98,7 @@ struct kmod_module {
>       * is set. There's nothing much useful one can do with such a
>       * "module", except knowing it's builtin.
>       */
> -    bool builtin : 1;
> +    enum kmod_builtin_status builtin;
>  };
>  
>  static inline const char *path_join(const char *path, size_t prefixlen,
> @@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod, bool visited)
>      mod->visited = visited;
>  }
>  
> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
> +void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin)
>  {
>      mod->builtin = builtin;
>  }
> @@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod, bool required)
>      mod->required = required;
>  }
>  
> +bool kmod_module_is_builtin(const struct kmod_module *mod)
> +{
> +    int builtin = mod->builtin;
> +
> +    if (builtin == KMOD_BUILTIN_UNKNOWN)
> +        builtin = kmod_just_lookup_alias_from_builtin_file(mod->ctx, mod->name);
> +
> +    return mod->builtin == KMOD_BUILTIN_YES;
> +}
>  /*
>   * Memory layout with alias:
>   *
> @@ -924,7 +933,7 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
>                  module_is_blacklisted(mod))
>              continue;
>  
> -        if ((filter_type & KMOD_FILTER_BUILTIN) && mod->builtin)
> +        if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
>              continue;
>  
>          node = kmod_list_append(*output, mod);
> @@ -1713,7 +1722,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>      if (mod == NULL)
>          return -ENOENT;
>  
> -    if (mod->builtin)
> +    if (kmod_module_is_builtin(mod))
>          return KMOD_MODULE_BUILTIN;
>  
>      pathlen = snprintf(path, sizeof(path),
> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
> index 1a5a66b..d79bb12 100644
> --- a/libkmod/libkmod.c
> +++ b/libkmod/libkmod.c
> @@ -525,7 +525,7 @@ int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name,
>              goto finish;
>          }
>  
> -        kmod_module_set_builtin(mod, true);
> +        kmod_module_set_builtin(mod, KMOD_BUILTIN_YES);
>          *list = kmod_list_append(*list, mod);
>          if (*list == NULL)
>              err = -ENOMEM;
> @@ -536,6 +536,39 @@ finish:
>      return err;
>  }
>  
> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx,
> +        const char *name) {
> +    char *line = NULL;
> +
> +    if (ctx->indexes[KMOD_INDEX_MODULES_BUILTIN]) {
> +        DBG(ctx, "use mmaped index '%s' modname=%s\n",
> +                index_files[KMOD_INDEX_MODULES_BUILTIN].fn, name);
> +        line = index_mm_search(ctx->indexes[KMOD_INDEX_MODULES_BUILTIN], name);
> +    } else {
> +        struct index_file *idx;
> +        char fn[PATH_MAX];
> +
> +        snprintf(fn, sizeof(fn), "%s/%s.bin", ctx->dirname,
> +                index_files[KMOD_INDEX_MODULES_BUILTIN].fn);
> +        DBG(ctx, "file=%s modname=%s\n", fn, name);
> +
> +        idx = index_file_open(fn);
> +        if (idx == NULL) {
> +            DBG(ctx, "could not open builtin file '%s'\n", fn);
> +            goto finish;
> +        }
> +
> +        line = index_search(idx, name);
> +        index_file_close(idx);
> +    }
> +
> +    if (line != NULL)
> +        return KMOD_BUILTIN_YES;
> +
> +finish:
> +        return KMOD_BUILTIN_NO;
> +}
> +
>  char *kmod_search_moddep(struct kmod_ctx *ctx, const char *name)
>  {
>      struct index_file *idx;
>
>
>


I guess there are some mistakes

1) return mod->builtin == KMOD_BUILTIN_YES;  should be made to
return builtin == KMOD_BUILTIN_YES;
2) S_ISDIR check needs to be removed.

Lucas,
In any case , Can this patch can be taken in sequence?
My first original patch of removing directory check can be taken first. Then the necessary changes required. (you might add as well in your free time as suggested by you)

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Lucas De Marchi]

On Thu, Feb 19, 2015 at 12:35 PM, Harish Jenny Kandiga Nagaraj
<harish_kandiga@mentor.com> wrote:
>
> On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
>> On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
>>> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
>> diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
>> index 417f232..f6ffd3e 100644
>> --- a/libkmod/libkmod-internal.h
>> +++ b/libkmod/libkmod-internal.h
>> @@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
>>  #  endif
>>  #endif
>>
>> +/* Flags to kmod_builtin_status() */
>> +enum kmod_builtin_status {
>> +    KMOD_BUILTIN_UNKNOWN = 0,
>> +    KMOD_BUILTIN_NO = 1,
>> +    KMOD_BUILTIN_YES = 2
>> +};
>> +
>>  void kmod_log(const struct kmod_ctx *ctx,
>>          int priority, const char *file, int line, const char *fn,
>>          const char *format, ...) __attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));
>> @@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx *ctx, const char *name,
>>  int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>  int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>  int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name)__attribute__((nonnull(1, 2)));
>>  int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>  void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited) __attribute__((nonnull((1))));
>>  void kmod_set_modules_required(struct kmod_ctx *ctx, bool required) __attribute__((nonnull((1))));
>> @@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod, char *line) __attribute__
>>  void kmod_module_set_install_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
>>  void kmod_module_set_remove_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
>>  void kmod_module_set_visited(struct kmod_module *mod, bool visited) __attribute__((nonnull(1)));
>> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin) __attribute__((nonnull((1))));
>> +void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin) __attribute__((nonnull((1))));
>>  void kmod_module_set_required(struct kmod_module *mod, bool required) __attribute__((nonnull(1)));
>> -
>> +bool kmod_module_is_builtin(const struct kmod_module *mod);
>>
>>  /* libkmod-file.c */
>>  struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filename) _must_check_ __attribute__((nonnull(1,2)));
>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index 19bb2ed..6b2f2f1 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -98,7 +98,7 @@ struct kmod_module {
>>       * is set. There's nothing much useful one can do with such a
>>       * "module", except knowing it's builtin.
>>       */
>> -    bool builtin : 1;
>> +    enum kmod_builtin_status builtin;
>>  };
>>
>>  static inline const char *path_join(const char *path, size_t prefixlen,
>> @@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod, bool visited)
>>      mod->visited = visited;
>>  }
>>
>> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>> +void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin)
>>  {
>>      mod->builtin = builtin;
>>  }
>> @@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod, bool required)
>>      mod->required = required;
>>  }
>>
>> +bool kmod_module_is_builtin(const struct kmod_module *mod)
>> +{
>> +    int builtin = mod->builtin;
>> +
>> +    if (builtin == KMOD_BUILTIN_UNKNOWN)
>> +        builtin = kmod_just_lookup_alias_from_builtin_file(mod->ctx, mod->name);
>> +
>> +    return mod->builtin == KMOD_BUILTIN_YES;
>> +}
>>  /*
>>   * Memory layout with alias:
>>   *
>> @@ -924,7 +933,7 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
>>                  module_is_blacklisted(mod))
>>              continue;
>>
>> -        if ((filter_type & KMOD_FILTER_BUILTIN) && mod->builtin)
>> +        if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
>>              continue;
>>
>>          node = kmod_list_append(*output, mod);
>> @@ -1713,7 +1722,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>>      if (mod == NULL)
>>          return -ENOENT;
>>
>> -    if (mod->builtin)
>> +    if (kmod_module_is_builtin(mod))
>>          return KMOD_MODULE_BUILTIN;
>>
>>      pathlen = snprintf(path, sizeof(path),
>> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
>> index 1a5a66b..d79bb12 100644
>> --- a/libkmod/libkmod.c
>> +++ b/libkmod/libkmod.c
>> @@ -525,7 +525,7 @@ int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name,
>>              goto finish;
>>          }
>>
>> -        kmod_module_set_builtin(mod, true);
>> +        kmod_module_set_builtin(mod, KMOD_BUILTIN_YES);
>>          *list = kmod_list_append(*list, mod);
>>          if (*list == NULL)
>>              err = -ENOMEM;
>> @@ -536,6 +536,39 @@ finish:
>>      return err;
>>  }
>>
>> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx,
>> +        const char *name) {
>> +    char *line = NULL;
>> +
>> +    if (ctx->indexes[KMOD_INDEX_MODULES_BUILTIN]) {
>> +        DBG(ctx, "use mmaped index '%s' modname=%s\n",
>> +                index_files[KMOD_INDEX_MODULES_BUILTIN].fn, name);
>> +        line = index_mm_search(ctx->indexes[KMOD_INDEX_MODULES_BUILTIN], name);
>> +    } else {
>> +        struct index_file *idx;
>> +        char fn[PATH_MAX];
>> +
>> +        snprintf(fn, sizeof(fn), "%s/%s.bin", ctx->dirname,
>> +                index_files[KMOD_INDEX_MODULES_BUILTIN].fn);
>> +        DBG(ctx, "file=%s modname=%s\n", fn, name);
>> +
>> +        idx = index_file_open(fn);
>> +        if (idx == NULL) {
>> +            DBG(ctx, "could not open builtin file '%s'\n", fn);
>> +            goto finish;
>> +        }
>> +
>> +        line = index_search(idx, name);
>> +        index_file_close(idx);
>> +    }
>> +
>> +    if (line != NULL)
>> +        return KMOD_BUILTIN_YES;
>> +
>> +finish:
>> +        return KMOD_BUILTIN_NO;
>> +}
>> +
>>  char *kmod_search_moddep(struct kmod_ctx *ctx, const char *name)
>>  {
>>      struct index_file *idx;
>>
>>
>>
>
>
> I guess there are some mistakes
>
> 1) return mod->builtin == KMOD_BUILTIN_YES;  should be made to
> return builtin == KMOD_BUILTIN_YES;
> 2) S_ISDIR check needs to be removed.
>
> Lucas,
> In any case , Can this patch can be taken in sequence?
> My first original patch of removing directory check can be taken first. Then the necessary changes required. (you might add as well in your free time as suggested by you)

I fixed some mistakes like you pointed out, added minor changes and
applied to the master branch:
https://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

I also added some tests to the testsuite on top. Could you please take
a look if everything is right for your use case in master branch?

thanks

-- 
Lucas De Marchi

Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN

[Harish Jenny Kandiga Nagaraj]

On Saturday 28 February 2015 10:58 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 12:35 PM, Harish Jenny Kandiga Nagaraj
> <harish_kandiga@mentor.com> wrote:
>> On Thursday 19 February 2015 07:32 PM, Harish Jenny Kandiga Nagaraj wrote:
>>> On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
>>>> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
>>> diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
>>> index 417f232..f6ffd3e 100644
>>> --- a/libkmod/libkmod-internal.h
>>> +++ b/libkmod/libkmod-internal.h
>>> @@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
>>>  #  endif
>>>  #endif
>>>
>>> +/* Flags to kmod_builtin_status() */
>>> +enum kmod_builtin_status {
>>> +    KMOD_BUILTIN_UNKNOWN = 0,
>>> +    KMOD_BUILTIN_NO = 1,
>>> +    KMOD_BUILTIN_YES = 2
>>> +};
>>> +
>>>  void kmod_log(const struct kmod_ctx *ctx,
>>>          int priority, const char *file, int line, const char *fn,
>>>          const char *format, ...) __attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));
>>> @@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx *ctx, const char *name,
>>>  int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>>  int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>>  int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name)__attribute__((nonnull(1, 2)));
>>>  int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
>>>  void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited) __attribute__((nonnull((1))));
>>>  void kmod_set_modules_required(struct kmod_ctx *ctx, bool required) __attribute__((nonnull((1))));
>>> @@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod, char *line) __attribute__
>>>  void kmod_module_set_install_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
>>>  void kmod_module_set_remove_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
>>>  void kmod_module_set_visited(struct kmod_module *mod, bool visited) __attribute__((nonnull(1)));
>>> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin) __attribute__((nonnull((1))));
>>> +void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin) __attribute__((nonnull((1))));
>>>  void kmod_module_set_required(struct kmod_module *mod, bool required) __attribute__((nonnull(1)));
>>> -
>>> +bool kmod_module_is_builtin(const struct kmod_module *mod);
>>>
>>>  /* libkmod-file.c */
>>>  struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filename) _must_check_ __attribute__((nonnull(1,2)));
>>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>>> index 19bb2ed..6b2f2f1 100644
>>> --- a/libkmod/libkmod-module.c
>>> +++ b/libkmod/libkmod-module.c
>>> @@ -98,7 +98,7 @@ struct kmod_module {
>>>       * is set. There's nothing much useful one can do with such a
>>>       * "module", except knowing it's builtin.
>>>       */
>>> -    bool builtin : 1;
>>> +    enum kmod_builtin_status builtin;
>>>  };
>>>
>>>  static inline const char *path_join(const char *path, size_t prefixlen,
>>> @@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod, bool visited)
>>>      mod->visited = visited;
>>>  }
>>>
>>> -void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>>> +void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin)
>>>  {
>>>      mod->builtin = builtin;
>>>  }
>>> @@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod, bool required)
>>>      mod->required = required;
>>>  }
>>>
>>> +bool kmod_module_is_builtin(const struct kmod_module *mod)
>>> +{
>>> +    int builtin = mod->builtin;
>>> +
>>> +    if (builtin == KMOD_BUILTIN_UNKNOWN)
>>> +        builtin = kmod_just_lookup_alias_from_builtin_file(mod->ctx, mod->name);
>>> +
>>> +    return mod->builtin == KMOD_BUILTIN_YES;
>>> +}
>>>  /*
>>>   * Memory layout with alias:
>>>   *
>>> @@ -924,7 +933,7 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
>>>                  module_is_blacklisted(mod))
>>>              continue;
>>>
>>> -        if ((filter_type & KMOD_FILTER_BUILTIN) && mod->builtin)
>>> +        if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
>>>              continue;
>>>
>>>          node = kmod_list_append(*output, mod);
>>> @@ -1713,7 +1722,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>>>      if (mod == NULL)
>>>          return -ENOENT;
>>>
>>> -    if (mod->builtin)
>>> +    if (kmod_module_is_builtin(mod))
>>>          return KMOD_MODULE_BUILTIN;
>>>
>>>      pathlen = snprintf(path, sizeof(path),
>>> diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
>>> index 1a5a66b..d79bb12 100644
>>> --- a/libkmod/libkmod.c
>>> +++ b/libkmod/libkmod.c
>>> @@ -525,7 +525,7 @@ int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name,
>>>              goto finish;
>>>          }
>>>
>>> -        kmod_module_set_builtin(mod, true);
>>> +        kmod_module_set_builtin(mod, KMOD_BUILTIN_YES);
>>>          *list = kmod_list_append(*list, mod);
>>>          if (*list == NULL)
>>>              err = -ENOMEM;
>>> @@ -536,6 +536,39 @@ finish:
>>>      return err;
>>>  }
>>>
>>> +int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx,
>>> +        const char *name) {
>>> +    char *line = NULL;
>>> +
>>> +    if (ctx->indexes[KMOD_INDEX_MODULES_BUILTIN]) {
>>> +        DBG(ctx, "use mmaped index '%s' modname=%s\n",
>>> +                index_files[KMOD_INDEX_MODULES_BUILTIN].fn, name);
>>> +        line = index_mm_search(ctx->indexes[KMOD_INDEX_MODULES_BUILTIN], name);
>>> +    } else {
>>> +        struct index_file *idx;
>>> +        char fn[PATH_MAX];
>>> +
>>> +        snprintf(fn, sizeof(fn), "%s/%s.bin", ctx->dirname,
>>> +                index_files[KMOD_INDEX_MODULES_BUILTIN].fn);
>>> +        DBG(ctx, "file=%s modname=%s\n", fn, name);
>>> +
>>> +        idx = index_file_open(fn);
>>> +        if (idx == NULL) {
>>> +            DBG(ctx, "could not open builtin file '%s'\n", fn);
>>> +            goto finish;
>>> +        }
>>> +
>>> +        line = index_search(idx, name);
>>> +        index_file_close(idx);
>>> +    }
>>> +
>>> +    if (line != NULL)
>>> +        return KMOD_BUILTIN_YES;
>>> +
>>> +finish:
>>> +        return KMOD_BUILTIN_NO;
>>> +}
>>> +
>>>  char *kmod_search_moddep(struct kmod_ctx *ctx, const char *name)
>>>  {
>>>      struct index_file *idx;
>>>
>>>
>>>
>>
>> I guess there are some mistakes
>>
>> 1) return mod->builtin == KMOD_BUILTIN_YES;  should be made to
>> return builtin == KMOD_BUILTIN_YES;
>> 2) S_ISDIR check needs to be removed.
>>
>> Lucas,
>> In any case , Can this patch can be taken in sequence?
>> My first original patch of removing directory check can be taken first. Then the necessary changes required. (you might add as well in your free time as suggested by you)
> I fixed some mistakes like you pointed out, added minor changes and
> applied to the master branch:
> https://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4
>
> I also added some tests to the testsuite on top. Could you please take
> a look if everything is right for your use case in master branch?
>
> thanks
>

checked the master branch for the commit. It is right for our use case.

Thanks,
Harish