Re: [PATCH v2] tools: add static-nodes tool

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

 



On Tue, Apr 16, 2013 at 1:20 AM, Lucas De Marchi
<lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> Hi Tom,

Hi Lucas,

Thanks for your review!

> On Fri, Apr 12, 2013 at 9:15 PM, Tom Gundersen <teg@xxxxxxx> wrote:
>> This tool reads modules.devname from the current kernel directory and outputs
>> the information.
>>
>> For now only the tmpfiles.d(5) format is supported, but more could easily be
>> added in the future if there is a need.
>>
>> When booting with systemd, the new tool is called at boot to instruct
>> systemd-tmpfiles to create the necessary static modules before starting
>> systemd-udevd.
>>
>> This means nothing but kmod needs to reads the private files under /lib/modules/.
>
> For me this would be the main goal of this patch or anything similar to this.

Yeah, I agree.

>> Cc: <linux-hotplug@xxxxxxxxxxxxxxx>
>> Cc: <systemd-devel@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> v2: adressed concerns raised by Dave, and made the tool a bit more generic so
>> more output formats may be added in the future as suggested by Lucas. Also
>> included the systemd unit file to hook this up with systemd.
>>
>>  Makefile.am                        |  20 ++++-
>>  configure.ac                       |   8 ++
>>  tools/kmod-static-nodes.service.in |  16 ++++
>>  tools/kmod.c                       |   1 +
>>  tools/kmod.h                       |   1 +
>>  tools/static-nodes.c               | 163 +++++++++++++++++++++++++++++++++++++
>>  6 files changed, 207 insertions(+), 2 deletions(-)
>>  create mode 100644 tools/kmod-static-nodes.service.in
>>  create mode 100644 tools/static-nodes.c
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 9feaf96..333e861 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -36,6 +36,9 @@ SED_PROCESS = \
>>  %.pc: %.pc.in Makefile
>>         $(SED_PROCESS)
>>
>> +%.service: %.service.in Makefile
>> +       $(SED_PROCESS)
>> +
>>  LIBKMOD_CURRENT=4
>>  LIBKMOD_REVISION=2
>>  LIBKMOD_AGE=2
>> @@ -88,6 +91,17 @@ pkgconfig_DATA = libkmod/libkmod.pc
>>  EXTRA_DIST += libkmod/libkmod.pc.in
>>  CLEANFILES += libkmod/libkmod.pc
>>
>> +if HAVE_SYSTEMD
>> +systemdsystemunit_DATA = tools/kmod-static-nodes.service
>> +EXTRA_DIST += tools/kmod-static-nodes.service.in
>> +CLEANFILES += tools/kmod-static-nodes.service
>> +
>> +install-data-hook:
>> +       $(MKDIR_P) $(DESTDIR)$(systemdsystemunitdir)/sysinit.target.wants
>> +       ln -sf ../kmod-static-nodes.service \
>> +               $(DESTDIR)$(systemdsystemunitdir)/sysinit.target.wants/kmod-static-nodes.service
>> +endif
>> +
>>  install-exec-hook:
>>         if test "$(libdir)" != "$(rootlibdir)"; then \
>>                 $(MKDIR_P) $(DESTDIR)$(rootlibdir) && \
>> @@ -109,7 +123,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/static-nodes.c
>>  tools_kmod_LDADD = libkmod/libkmod-util.la \
>>                    libkmod/libkmod.la
>>
>> @@ -211,7 +226,8 @@ testsuite-distclean:
>>  DISTCLEAN_LOCAL_HOOKS += testsuite-distclean
>>  EXTRA_DIST += testsuite/rootfs-pristine
>>
>> -DISTCHECK_CONFIGURE_FLAGS=--enable-gtk-doc --sysconfdir=/etc --with-zlib
>> +DISTCHECK_CONFIGURE_FLAGS=--enable-gtk-doc --sysconfdir=/etc --with-zlib \
>> +                         --with-systemdsystemunitdir=$$dc_install_base/$(systemdsystemunitdir)
>>
>>  distclean-local: $(DISTCLEAN_LOCAL_HOOKS)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 566b317..af5ed52 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -76,6 +76,13 @@ AS_IF([test "x$with_zlib" != "xno"], [
>>         AC_MSG_NOTICE([zlib support not requested])
>>  ])
>>
>> +AC_ARG_WITH([systemdsystemunitdir],
>> +            AS_HELP_STRING([--with-systemdsystemunitdir=DIR], [Directory for systemd service files]),
>> +            [], [with_systemdsystemunitdir=$($PKG_CONFIG --variable=systemdsystemunitdir systemd)])
>> +if test "x$with_systemdsystemunitdir" != xno; then
>> +        AC_SUBST([systemdsystemunitdir], [$with_systemdsystemunitdir])
>> +fi
>> +AM_CONDITIONAL(HAVE_SYSTEMD, [test -n "$with_systemdsystemunitdir" -a "x$with_systemdsystemunitdir" != xno ])
>>
>>  #####################################################################
>>  # --enable-
>> @@ -200,6 +207,7 @@ AC_MSG_RESULT([
>>         compiler:               ${CC}
>>         cflags:                 ${with_cflags} ${CFLAGS}
>>         ldflags:                ${with_ldflags} ${LDFLAGS}
>> +       systemdsystemunitdir:   ${with_systemdsystemunitdir}
>>
>>         tools:                  ${enable_tools}
>>         logging:                ${enable_logging}
>> diff --git a/tools/kmod-static-nodes.service.in b/tools/kmod-static-nodes.service.in
>> new file mode 100644
>> index 0000000..be8482e
>> --- /dev/null
>> +++ b/tools/kmod-static-nodes.service.in
>
> I'm not sure we want the service file to live in kmod repository.
> Maybe we could let this in systemd and in kmod we only add the support
> to create this type of information?

I don't really mind. For now I'll drop it, and we can agree on where
to put it later.

>> @@ -0,0 +1,16 @@
>> +#  This file is part of kmod.
>> +#
>> +#  kmod is free software; you can redistribute it and/or modify it
>> +#  under the terms of the GNU Lesser General Public License as published by
>> +#  the Free Software Foundation; either version 2.1 of the License, or
>> +#  (at your option) any later version.
>> +
>> +[Unit]
>> +Description=Create list of static nodes to be created in /dev
>> +DefaultDependencies=no
>> +Before=sysinit.target systemd-static-nodes.service
>> +
>> +[Service]
>> +Type=oneshot
>> +ExecStart=/bin/mkdir -p /run/tmpfiles.d
>> +ExecStart=@prefix@/bin/kmod static-nodes tmpfiles --output=/run/tmpfiles.d/kmod.conf
>
> The syntax is a bit cumbersome too, but since we still don't have any
> command (except from the test-only "list") I think we could let this
> as is and change later, before a new release.
>
> IMO it should be kmod static-nodes --format=...
>
> It's ok to implement only the tmpfiles output, though the default
> should be an human readable format, just like git commands are.

Thanks for the suggestions, makes sense. I resent with these changes.

>> diff --git a/tools/kmod.c b/tools/kmod.c
>> index ebb8875..347bb7d 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_static_nodes,
>>  };
>>
>>  static const struct kmod_cmd *kmod_compat_cmds[] = {
>> diff --git a/tools/kmod.h b/tools/kmod.h
>> index 80fa4c2..68a646a 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_static_nodes;
>>
>>  #include "log.h"
>> diff --git a/tools/static-nodes.c b/tools/static-nodes.c
>> new file mode 100644
>> index 0000000..a79fc3d
>> --- /dev/null
>> +++ b/tools/static-nodes.c
>> @@ -0,0 +1,163 @@
>> +/*
>> + * kmod-static-nodes - manage modules.devname
>> + *
>> + * Copyright (C) 2004-2012 Kay Sievers <kay@xxxxxxxx>
>> + * Copyright (C) 2011-2013  ProFUSION embedded systems
>> + * Copyright (C) 2013 Tom Gundersen <teg@xxxxxxx>
>> + *
>> + * 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 <getopt.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-util.h"
>> +
>> +#include "kmod.h"
>> +
>> +static const char cmdopts_s[] = "o:h";
>> +static const struct option cmdopts[] = {
>> +        { "output", required_argument, 0, 'o'},
>> +        { "help", no_argument, 0, 'h'},
>> +        { },
>> +};
>> +
>> +static void help(void)
>> +{
>> +        printf("Usage:\n"
>> +                "\t%s static-nodes format [options]\n"
>> +                "\n"
>> +                "kmod static-nodes outputs the static-node information of the currently running kernel.\n"
>> +                "\n"
>> +                "Options:\n"
>> +                "\t-o, --output=FILE  write output to file\n"
>> +                "\t-h, --help         show this help\n"
>> +                "\n"
>> +                "Formats:\n"
>> +                "  tmpfiles    the tmpfiles.d(5) used by systemd-tmpfiles.\n",
>> +                program_invocation_short_name);
>> +}
>> +
>> +static int write_tmpfile(FILE *in, FILE *out) {
>> +        char buf[4096];
>> +        int ret = EXIT_SUCCESS;
>> +
>> +        while (fgets(buf, sizeof(buf), in) != NULL) {
>> +                char devname[PATH_MAX];
>> +                char type;
>> +                unsigned int maj, min;
>> +                int matches;
>> +
>> +                if (buf[0] == '#')
>> +                        continue;
>> +
>> +                matches = sscanf(buf, "%*s %s %c%u:%u", devname, &type, &maj, &min);
>> +                if (matches != 4 || (type != 'c' && type != 'b')) {
>> +                        fprintf(stderr, "Error: invalid devname entry: %s", buf);
>> +                        ret = EXIT_FAILURE;
>> +                        continue;
>> +                }
>> +
>> +                fprintf(out, "%c /dev/%s 0600 - - - %u:%u\n", type, devname, maj, min);
>> +        }
>> +
>> +        return ret;
>> +}
>> +
>> +static int do_static_nodes(int argc, char *argv[])
>> +{
>> +        struct utsname kernel;
>> +        char modules[PATH_MAX];
>> +        FILE *in = NULL, *out = stdout;
>> +        int ret = EXIT_SUCCESS;
>> +
>> +        for (;;) {
>> +                int c, idx = 0;
>> +
>> +                c = getopt_long(argc - 1, argv + 1, cmdopts_s, cmdopts, &idx);
>> +                if (c == -1) {
>> +                        break;
>> +                }
>> +                switch (c) {
>> +                case 'o':
>> +                        out = fopen(optarg, "we");
>> +                        if (out == NULL) {
>> +                                fprintf(stderr, "Error: could not create %s!\n", optarg);
>> +                                ret = EXIT_FAILURE;
>> +                                goto finish;
>> +                        }
>> +                        break;
>> +                case 'h':
>> +                        help();
>> +                        goto finish;
>> +                case '?':
>> +                        ret = EXIT_FAILURE;
>> +                        goto finish;
>> +                default:
>> +                        fprintf(stderr, "Unexpected commandline option '%c'.\n", c);
>> +                        help();
>> +                        ret = EXIT_FAILURE;
>> +                        goto finish;
>> +                }
>> +        }
>> +
>> +        if (argc < 2) {
>> +                help();
>> +                ret = EXIT_FAILURE;
>> +                goto finish;
>> +        }
>> +
>> +        if (!streq(argv[1], "tmpfiles")) {
>> +                fprintf(stderr, "Unknown format: '%s'.\n", argv[1]);
>> +                help();
>> +                ret = EXIT_FAILURE;
>> +                goto finish;
>> +        }
>> +
>> +        if (uname(&kernel) < 0) {
>> +                fputs("Error: uname failed!\n", stderr);
>> +                ret = EXIT_FAILURE;
>> +                goto finish;
>> +        }
>> +        snprintf(modules, sizeof(modules), "/lib/modules/%s/modules.devname", kernel.release);
>> +        in = fopen(modules, "re");
>> +        if (in == NULL && errno != ENOENT) {
>> +                fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname!\n", kernel.release);
>> +                ret = EXIT_FAILURE;
>> +                goto finish;
>> +        }
>> +
>> +        ret = write_tmpfile(in, out);
>> +
>> +finish:
>> +        if (in)
>> +                fclose(in);
>> +        if (out)
>> +                fclose(out);
>> +        return ret;
>> +}
>> +
>> +const struct kmod_cmd kmod_cmd_static_nodes = {
>> +       .name = "static-nodes",
>> +       .cmd = do_static_nodes,
>> +       .help = "outputs the static-node information of the currently running kernel",
>> +};
>> --
>
>
> Lucas De Marchi
--
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