[PATCH] [WIP]tools: add devname2tmpfile

Dave Reisner

2013-04-11 18:21:02 UTC

Post by Tom Gundersen
This tool reads modules.devname from the current kernel directory and writes the information
out in the tmpfiles format so that systemd-tmpfiles can use this to create the required device
nodes before systemd-udevd is started on boot.

I'm a little confused as to why this should live in kmod if the output
is only useful to systemd. Rather, I would have suspected that this
would be part of src/core/kmod-setup.c in systemd. modules.devname is
easily parseable and requires no knowledge of what kmod does internally.
In fact, your code initializes a kmod context but then never uses it for
anything.

I've left some comments for you regardless of where this ends up
living...

Post by Tom Gundersen
This makes sure nothing but kmod reads the private files under
/usr/lib/modules/.

The code says otherwise -- it reads from /lib/modules/...

Post by Tom Gundersen
---
Makefile.am | 3 +-
tools/devname2tmpfile.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++
tools/kmod.c | 1 +
tools/kmod.h | 1 +
4 files changed, 130 insertions(+), 1 deletion(-)
create mode 100644 tools/devname2tmpfile.c
diff --git a/Makefile.am b/Makefile.am
index 9feaf96..50cd380 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -109,7 +109,8 @@ noinst_SCRIPTS = tools/insmod tools/rmmod tools/lsmod \
tools_kmod_SOURCES = tools/kmod.c tools/kmod.h tools/lsmod.c \
tools/rmmod.c tools/insmod.c \
tools/modinfo.c tools/modprobe.c \
- tools/depmod.c tools/log.h tools/log.c
+ tools/depmod.c tools/log.h tools/log.c \
+ tools/devname2tmpfile.c
tools_kmod_LDADD = libkmod/libkmod-util.la \
libkmod/libkmod.la
diff --git a/tools/devname2tmpfile.c b/tools/devname2tmpfile.c
new file mode 100644
index 0000000..05a2b4e
--- /dev/null
+++ b/tools/devname2tmpfile.c
@@ -0,0 +1,126 @@
+/*
+ * kmod-devname2tmpfile - write devnames of current kernel in tmpfiles.d format
+ *
+ * Copyright (C) 2011-2013 ProFUSION embedded systems
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stddef.h>
+#include <errno.h>
+#include <unistd.h>
+#include <string.h>
+#include <limits.h>
+#include <sys/utsname.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include "libkmod.h"
+
+#include "kmod.h"
+
+static int do_devname2tmpfile(int argc, char *argv[])
+{
+ struct kmod_ctx *ctx;
+ struct utsname kernel;
+ const char *null_config = NULL;
+ char modules[PATH_MAX];
+ char buf[4096];
+ FILE *f, *out;
+
+ if (argc != 1) {
+ fprintf(stderr, "Usage: %s\n", argv[0]);
+ return EXIT_FAILURE;
+ }
+
+ ctx = kmod_new(NULL, &null_config);

As mentioned above, this is never actually used, but it seems the only
thing it could be potentially used for is logging, rather than writing
directly to a stream.

Post by Tom Gundersen
+ if (ctx == NULL) {
+ fputs("Error: kmod_new() failed!\n", stderr);
+ return EXIT_FAILURE;
+ }
+
+ if (uname(&kernel) < 0) {
+ fputs("Error: uname failed!\n", stderr);
+ return EXIT_FAILURE;
+ }
+ snprintf(modules, sizeof(modules), "/lib/modules/%s/modules.devname", kernel.release);
+ f = fopen(modules, "re");
+ if (f == NULL) {
+ fprintf(stderr, "Error: could not open %s/modules.devname!\n", kernel.release);

Full path here instead of just a trailing fragment?

Post by Tom Gundersen
+ return EXIT_FAILURE;

I disagree that this should be an error or a failure.

Post by Tom Gundersen
+ }
+
+ if (mkdir("/run/tmpfiles.d/", 755) != 0 && errno != EEXIST) {
+ fputs("Error: /run/tmpfiles.d does not exist and could not be created!\n", stderr);
+ return EXIT_FAILURE;
+ }
+
+ out = fopen("/run/tmpfiles.d/kmod.conf", "we");
+ if (out == NULL) {
+ fputs("Error: could not create /run/tmpfiles.d/kmod.conf!\n", stderr);
+ return EXIT_FAILURE;
+ }

You appear to have some inconsistent whitespace above here in the
fprintf and fputs calls. There's more below which I haven't pointed out.

Post by Tom Gundersen
+
+ while (fgets(buf, sizeof(buf), f) != NULL) {
+ char *s;
+ const char *modname;
+ const char *devname;
+ const char *devno;
+ int maj, min;

You declare these as int (signed), but then treat them as unsigned (%u)
in sscanf and fprintf.

Post by Tom Gundersen
+ char type;
+
+ if (buf[0] == '#')
+ continue;
+
+ modname = buf;
+ s = strchr(modname, ' ');
+ if (s == NULL)
+ continue;
+ s[0] = '\0';
+
+ devname = &s[1];
+ s = strchr(devname, ' ');
+ if (s == NULL)
+ continue;
+ s[0] = '\0';
+
+ devno = &s[1];
+ s = strchr(devno, ' ');
+ if (s == NULL)
+ s = strchr(devno, '\n');
+ if (s != NULL)
+ s[0] = '\0';
+ if (sscanf(devno, "%c%u:%u", &type, &maj, &min) != 3)
+ continue;

This whole section is unnecessarily verbose. This should be a well
formed file where non-comment data matches the format string "%ms %ms
%c%u:%u". You can simply do your validation on that (i.e. sscanf
returns exactly 5). Note that %ms will allocate memory for the data, so
you need to free it after printing it below. Alternatively, just use
adequately sized stack allocated char[] with %s.

Post by Tom Gundersen
+
+ if (type != 'c' && type != 'b')
+ continue;

I think this is worth logging about given that depmod currently never
writes anything but 'b' or 'c' here. I suspect this won't be changing
any time soon...

Post by Tom Gundersen
+ fprintf(out, "%c /dev/%s 0600 - - - %u:%u\n", type, devname, maj, min);
+ }
+
+ fclose(f);
+ kmod_unref(ctx);
+
+ return EXIT_SUCCESS;
+}
+
+const struct kmod_cmd kmod_cmd_devname2tmpfile = {
+ .name = "devname2tmpfile",
+ .cmd = do_devname2tmpfile,
+ .help = "write devnames of current kernel in tmpfiles.d format",
+};
diff --git a/tools/kmod.c b/tools/kmod.c
index ebb8875..ff6518c 100644
--- a/tools/kmod.c
+++ b/tools/kmod.c
@@ -37,6 +37,7 @@ static const struct kmod_cmd kmod_cmd_help;
static const struct kmod_cmd *kmod_cmds[] = {
&kmod_cmd_help,
&kmod_cmd_list,
+ &kmod_cmd_devname2tmpfile,
};
static const struct kmod_cmd *kmod_compat_cmds[] = {
diff --git a/tools/kmod.h b/tools/kmod.h
index 80fa4c2..6410ed5 100644
--- a/tools/kmod.h
+++ b/tools/kmod.h
@@ -35,5 +35,6 @@ extern const struct kmod_cmd kmod_cmd_compat_modprobe;
extern const struct kmod_cmd kmod_cmd_compat_depmod;
extern const struct kmod_cmd kmod_cmd_list;
+extern const struct kmod_cmd kmod_cmd_devname2tmpfile;
#include "log.h"
--
1.8.2.1
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
More majordomo info at http://vger.kernel.org/majordomo-info.html