Re: module-init-tools: [GIT v2] softdep command

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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

[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux