Thanks! Pushed a new version of the patch fixing the minor issues.
On Fri, Mar 06, 2015 at 12:23:29AM -0300, Lucas De Marchi wrote:
> Overall it's nice, but I have some comments on this one. See below
> > +DEFINE_TEST(kmod_tool_insert,
> > + .description = "check kmod insert",
> > + .config = {
> > + [TC_UNAME_R] = "4.4.4",
> > + [TC_ROOTFS] = TESTSUITE_ROOTFS "test-tools/insert",
> > + [TC_INIT_MODULE_RETCODES] = "",
>
> are these and other similar ones correct? why do you need to set it
> to an empty string?
If TC_INIT_MODULE_RETCODES is NULL, the preload for init_module won't
be loaded, so we need it set.
When set, it will automatically succeed loading any non-builtin
modules, except those mentioned in the string, that will succeed or
not depending on the error code.
Same approach is taken by TC_DELETE_MODULE_RETCODES.
> > + kmod_list_foreach(l, list) {
> > + struct kmod_module *mod = kmod_module_get_module(l);
> > +
> > + err = kmod_module_probe_insert_module(mod, KMOD_PROBE_APPLY_BLACKLIST, NULL, NULL, NULL, NULL);
> > + if (err != 0)
> > + ERR("Could not insert '%s': %s\n", kmod_module_get_name(mod), mod_strerror(err));
> > +
> > + kmod_module_unref(mod);
>
> missing 1 unref() in the error path. Just reorder the error check with
> the unref() and you'll be fine.
ERR() doesn't return or break, each get_module() will have a
corresponding unref() call.
I've ended up rewriting the return code handling to ensure that if one
of the insertions fail, return code will be EXIT_FAILURE.
Cheers,
Caio
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
