MODPROBE: next generation

[Denys Vlasenko]

On Tuesday 24 June 2008 23:48, dronnikov at gmail.com wrote:
> Hello, Denys!
> 
> Made a new patch. Tried to fix the issues you pointed out.

+	do {
+		/* search for the first char in word */
+		ptr = memchr(ptr, *word, len - (ptr - (char*)the_module));
+		if (ptr == NULL) /* no occurance left, done */
+			return NULL;
+		if (!strncmp(ptr, word, strlen(word))) {
+			ptr += strlen(word);
+			break;
+		}
+		++ptr;
+	} while (1);

Can you move strlen outside of the loop?


+	fprintf(fp, "\n");

Nitpicking department says "fputc".


+static void process_module(char *modules_dep, char *name
+	USE_FEATURE_MODPROBE_NG_OPTIONS_ON_CMDLINE(, const char *cmdline_options))
+{
...
+				process_module(modules_dep, s
+					USE_FEATURE_MODPROBE_NG_OPTIONS_ON_CMDLINE(, cmdline_options)
+				);

There is a neat trick how to make it readable. Put this before
the definition:

#if !ENABLE_FEATURE_MODPROBE_NG_OPTIONS_ON_CMDLINE
#define process_module(a, b, c) process_module(a, b)
#endif


+	// if depmod called ->
+	if ('d' == applet_name[0]) {
+		// force recreating modules.dep.bb
+		// N.B. to cope well-known "sendmail bug" we should ensure
+		// we are not writing to a symlink (to, e.g., /etc/passwd)
+		// placed by a malicious user in world-writable /tmp directory
+		// OTOH if we just unlink(DEPMOD_FILE) our depmod can kill smth unwillingly.
+		// So I prefer to bail out if depmod has to overwrite smth. Comments?
+		struct stat st;
+		if (!lstat(DEPMOD_FILE, &st)) {
+			if (!(option_mask32 & OPT_q)) {
+				// complain
+				errno = EEXIST;
+				bb_perror_msg_and_die(DEPMOD_FILE);
+			} else
+				goto quit;
+		}
+	}

I think depmod must create modules.dep.bb in /lib/modules, not in /tmp.
Just bail out if it can't create a file there.


+	if (!modules_dep) {
+		// depmod: dump modules definitions
+		// TODO: locking!!!
+		// TODO: locking!!!
+		// TODO: locking!!!
+		// TODO: locking!!!
+		// TODO: locking!!!
+		FILE *fp = fopen(DEPMOD_FILE, "w");
+		if (fp && recursive_action(".", 
+			ACTION_RECURSE, /* flags */
+			fileAction, /* file action */
+			NULL, /* dir action */
+			fp, /* user data */
+			0) /* depth */
+		) {
+			fprintf(fp, "\n"); // finalize definitions
+			fclose(fp);
+			// depmod was called? -> we're done
+			if ('d' == applet_name[0])
+				return EXIT_SUCCESS;
+			// read definitions
+			modules_dep = xmalloc_xopen_read_close(DEPMOD_FILE, NULL);
+		}
+	}

Locking can be provided by atomic rename. Roughly:

	// we are in /lib/modules[/X.Y.Z]
	template = xstrdup(DEPMOD_FILE "XXXXXX");
	fd = mkstemp(template);
	fp = fdopen(fd, "w");
	generate_modprobe_dep_bb(fp);
	fclose(fp); // closes fd too
	rename(template, DEPMOD_FILE); <=== atomically replaces modules.dep.bb

> Please, try it.

Will do in a minute.
--
vda