Messages in this thread Patch in this message | | | Date | Tue, 28 May 2019 15:21:22 -0700 | From | Lucas De Marchi <> | Subject | Re: [kmod][PATCH] module: fix error path in kmod_module_probe_insert_module() |
| |
On Sun, May 26, 2019 at 11:25:12AM +0200, Bartosz Golaszewski wrote: >From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > >The documentation says that kmod_module_probe_insert_module() will >return >0 if "stopped by a reason given in @flags" but it returns a >negative value if KMOD_PROBE_FAIL_ON_LOADED flag is passed and the >relevant module is already loaded. Fix the error path by using a >positive error value where required. > >Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >--- > libkmod/libkmod-module.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > >diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c >index bffe715..a3afaba 100644 >--- a/libkmod/libkmod-module.c >+++ b/libkmod/libkmod-module.c >@@ -1253,7 +1253,7 @@ KMOD_EXPORT int kmod_module_probe_insert_module(struct kmod_module *mod, > if (!(flags & KMOD_PROBE_IGNORE_LOADED) > && module_is_inkernel(mod)) { > if (flags & KMOD_PROBE_FAIL_ON_LOADED) >- return -EEXIST; >+ return EEXIST; > else > return 0; > } >@@ -1304,7 +1304,7 @@ KMOD_EXPORT int kmod_module_probe_insert_module(struct kmod_module *mod, > && module_is_inkernel(m)) { > DBG(mod->ctx, "Ignoring module '%s': already loaded\n", > m->name); >- err = -EEXIST; >+ err = EEXIST;
this would break the ABI. modprobe for instance, that is in the same repository would be broken by it:
kmod_list_foreach() { ... if (err >= 0) /* ignore flag return values such as a mod being blacklisted */ err = 0; else { switch (err) { case -EEXIST: ... }
I think we need to say the bug is actually in the bad documentation - my bad actually, on commit b1a51256a9ed ("libkmod-module: probe: change insertion to cover more use cases").
If you look at the definition of 'enum kmod_probe' you will see that not all flags are returned as positive values - the only values that have this behavior are the ones related to blacklist.
Could you rather patch the documentation?
thanks Lucas De Marchi
> goto finish_module; > } > >@@ -1340,14 +1340,14 @@ finish_module: > * been loaded between the check and the moment we try to > * insert it. > */ >- if (err == -EEXIST && m == mod && >+ if (err == EEXIST && m == mod && > (flags & KMOD_PROBE_FAIL_ON_LOADED)) > break; > > /* > * Ignore errors from softdeps > */ >- if (err == -EEXIST || !m->required) >+ if (err == EEXIST || !m->required) > err = 0; > > else if (err < 0) >-- >2.21.0 >
| |