> Anyway, care for some light weekend reading? > > The following changes since commit 9a1c905651a38d1f2c6d4d836f81b5e06a503521: > Jon Masters (1): > release: module-init-tools version 3.10 > > are available in the git repository at: > > git://github.com/andr345/module-init-tools.git modprobe-softdep > > or may be reviewed online at: > > http://github.com/andr345/module-init-tools/commits/modprobe-softdep > > Andreas Robinson (6): > modprobe: reduce nesting in conf parser > modprobe: trivial whitespace/indentation fixes in conf parser > modprobe: put configuration objects in a struct > modprobe: enable calling do_modprobe from within > handle_module,insmod,rmmod > modprobe: add softdep command parser > modprobe: add softdep command > > modprobe.c | 396 > +++++++++++++++++++++++++++++++++++++++++------------------- > 1 files changed, 274 insertions(+), 122 deletions(-) > > This is version 2, and like version 1 it's a basis for discussion and > not finished. > The first three commits are virtually unchanged since v1 and the > fourth is trivial, so it's safe to ignore them for now. > > Changes since v1: > > * The command core is properly implemented. find_softdep/do_softdep > mirror find_command/do_command, and should now deal with aliases. Great! This looks very functional. I have some style suggestions below, plus a fix for a memory leak on syntax errors. I can't help noticing you removed the loop detection. I suppose it's not needed now do_softdeps() is called in the same place as do_commands(). BTW, the recent fixes for loopy install commands will conflict with your work. It should be easy to resolve with git-merge, but I'm happy to help if you're not sure. > * The fork()-based workaround was removed. I pretend that the problem with > fatal() calls doesn't exist. It's a TODO, obviously. I think almost all of the fatal() calls are ok. I think it's fine to abort for NOFAIL allocations, missing or corrupt indexes, and missing configuration files. There are just two suspicious ones where load_strings() is called with fatal() as the error function. $ grep -n fatal modprobe.c index.c util.c elfops.c elfops_core.c tables.c modprobe.c:231: fatal("Module index is inconsistent\n"); modprobe.c:257: fatal("Could not load %s: %s\n", modules_dep_name, strerror(errno)); modprobe.c:305: fatal("New name %s is too long\n", newname); modprobe.c:347: tbl = module->ops->load_strings(module, ".modinfo", NULL, fatal); [[ This one needs looking at ]] modprobe.c:663: fatal("Could not load %s: %s\n", modules_dep_name, strerror(errno)); modprobe.c:978: fatal("Failed to open config file %s: %s\n", modprobe.c:1389: fatal("Module %s is not in kernel.\n", mod->modname); [[ This is conditional on the mit_first_time flag ]] modprobe.c:1569: errfn_t error = fatal; [[ This is in main(), so it doesn't affect your extra calls to do_modprobe() ]] modprobe.c:1676: fatal("Can't have multiple wildcards\n"); modprobe.c:1681: fatal("-t only supported with -l"); index.c:79: fatal("Module index: bad character '%c'=0x%x - only 7-bit ASCII is supported:" index.c:282: fatal("Module index: unexpected error: %s\n" return PERBIT(load_strings)(module, "__ksymtab_strings", symtbl, elfops_core.c:151: fatal); [[ This one needs looking at too ]] Patch nitpicks follow - @@ -1078,6 +1078,10 @@ static int insmod(struct list_head *list, const struct module_options *options, const struct module_command *commands, const char *cmdline_opts, + const char *configname, + const char *dirname, + const char *aliasfilename, + const char *symfilename, errfn_t error, modprobe_flags_t flags) I think you should omit the alias- and symfilename and calculate them as needed. You could change read_aliases_file() to take dirname, and a _relative_ filename which could then be a string literal ("modules.alias"). @@ -780,6 +791,37 @@ static int parse_config_file(const char *filename, *commands = add_command(underscores(modname), ptr, *commands); } + } else if (streq(cmd, "softdep")) { + char *tk; + int pre = 0, post = 0; + struct module_softdep deps = {}, *new; + + modname = underscores(strsep_skipspace(&ptr, "\t ")); + if (!modname || !ptr) + goto syntax_error; + deps.buf = line; + deps.modname = modname; + while ((tk = strsep_skipspace(&ptr, "\t ")) != NULL) { + if (streq(tk, "--pre")) { + pre = 1; post = 0; + } else if (streq(tk, "--post")) { + pre = 0; post = 1; + } else if (pre) { + deps.pre = NOFAIL( + strtbl_add(tk, deps.pre)); + } else if (post) { + deps.post = NOFAIL( + strtbl_add(tk, deps.post)); + } else + goto syntax_error; + } + new = NOFAIL(malloc(sizeof(deps))); + *new = deps; + new->next = conf->softdeps; + conf->softdeps = new; + + line = NULL; /* Don't free() this line. */ I think the lifecycle details could be a bit clearer if each field in "new" was explicitly assigned at the end. Also, it appears to leak the string tables on syntax errors. (warning, code edited in gmail and not tested :-): } else if (streq(cmd, "softdep")) { char *tk; int pre = 0, post = 0; struct string_table *pre_modnames = NULL; struct string_table *post_modnames = NULL; struct module_softdep *new; modname = underscores(strsep_skipspace(&ptr, "\t ")); if (!modname || !ptr) goto syntax_error; while ((tk = strsep_skipspace(&ptr, "\t ")) != NULL) { if (streq(tk, "--pre")) { pre = 1; post = 0; } else if (streq(tk, "--post")) { pre = 0; post = 1; } else if (pre) { pre_modnames = NOFAIL( strtbl_add(tk, pre_modnames)); } else if (post) { post_modnames = NOFAIL( strtbl_add(tk, post_modnames)); } else { strtbl_free(pre_modnames); strtbl_free(post_modnames); goto syntax_error; } } new = NOFAIL(malloc(sizeof(deps))); new->next = conf->softdeps; conf->softdeps = new; new->modname = modname; new->pre = pre_modnames; new->post = post_modnames; new->buf = line; line = NULL; /* Don't free() this line. */ } + /* Modprobe main module, passing cmdline_opts, ignoring softdep */ + + do_modprobe((char *) softdep->modname, NULL, (char *) cmdline_opts, + configname, dirname, aliasfilename, symfilename, + warn, flags | mit_ignore_commands); Ew, casting away const. We can avoid this by moving underscores() out of do_modprobe. Please put this on your TODO list if it's not there already :-). Regards Alan -- 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
