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

quoted

+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.

quoted

+       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