[PATCH 1/2] static-nodes: don't fail if modules.devname not found

[PATCH 1/2] static-nodes: don't fail if modules.devname not found

[Tom Gundersen]

In containers/VM's/initrd one might not have installed any modules and accompanying modules.devname
Don't fail if this is the case, just warn.

When used in systemd this means we don't get a failing unit on booting containers.
---
 tools/static-nodes.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/tools/static-nodes.c b/tools/static-nodes.c
index 96bb71e..b4a3c08 100644
--- a/tools/static-nodes.c
+++ b/tools/static-nodes.c
@@ -155,6 +155,7 @@ static int do_static_nodes(int argc, char *argv[])
 {
 	struct utsname kernel;
 	char modules[PATH_MAX];
+	char output[PATH_MAX];
 	FILE *in = NULL, *out = stdout;
 	const struct static_nodes_format *format = &static_nodes_format_human;
 	char buf[4096];
@@ -170,13 +171,8 @@ static int do_static_nodes(int argc, char *argv[])
 		}
 		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;
-			}
+			strncpy(output, optarg, PATH_MAX);
+			out = NULL;
 			break;
 		case 'f':
 			valid = 0;
@@ -221,12 +217,28 @@ static int do_static_nodes(int argc, char *argv[])
 		 kernel.release);
 	in = fopen(modules, "re");
 	if (in == NULL) {
-		fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname - %m\n",
-			kernel.release);
-		ret = EXIT_FAILURE;
+		if (errno == ENOENT) {
+			fprintf(stderr, "Warning: /lib/modules/%s/modules.devname not found - ignoring\n",
+				kernel.release);
+			ret = EXIT_SUCCESS;
+		} else {
+			fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname - %m\n",
+				kernel.release);
+			ret = EXIT_FAILURE;
+		}
 		goto finish;
 	}
 
+	if (out == NULL) {
+		out = fopen(output, "we");
+		if (out == NULL) {
+			fprintf(stderr, "Error: could not create %s!\n",
+				output);
+			ret = EXIT_FAILURE;
+			goto finish;
+		}
+	}
+
 	while (fgets(buf, sizeof(buf), in) != NULL) {
 		char modname[PATH_MAX];
 		char devname[PATH_MAX];
-- 
1.8.3.2

[PATCH 2/2] static-nodes: create parent directories of output file

[Tom Gundersen]

Allows us to drop call to "mkdir -p" from the systemd service file.
---
 Makefile.am          |  3 +-
 tools/mkdir.c        | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tools/mkdir.h        | 25 ++++++++++++++++
 tools/static-nodes.c | 14 ++++++++-
 4 files changed, 121 insertions(+), 2 deletions(-)
 create mode 100644 tools/mkdir.c
 create mode 100644 tools/mkdir.h

diff --git a/Makefile.am b/Makefile.am
index 0ed944c..ab49dfd 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -111,7 +111,8 @@ 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/static-nodes.c
+		     tools/static-nodes.c tools/mkdir.c \
+		     tools/mkdir.h
 tools_kmod_LDADD = libkmod/libkmod-util.la \
 		   libkmod/libkmod-internal.la
 
diff --git a/tools/mkdir.c b/tools/mkdir.c
new file mode 100644
index 0000000..9935366
--- /dev/null
+++ b/tools/mkdir.c
@@ -0,0 +1,81 @@
+/*
+ * Copyright (C) 2012  Lucas De Marchi <lucas.de.marchi@gmail.com
+ * Copyright (C) 2013  Tom Gundersen <teg@jklm.no>
+ *
+ * This program 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.
+ *
+ * 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include <errno.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include "mkdir.h"
+
+static int mkdir_p(char *path, mode_t mode)
+{
+	char *end = path + strlen(path);
+	struct stat st;
+	int r;
+
+	/*
+	 * don't try to overwrite existing file, succeed if it is a directory,
+	 * fail otherwise
+	 */
+	if (stat(path, &st) >= 0) {
+		if (S_ISDIR(st.st_mode)) {
+			return 0;
+		} else
+			return -ENOTDIR;
+	}
+
+	/* Find the last '/' */
+	while (end > path && *end != '/')
+		end--;
+
+	/* discard extra '/' */
+	while (end > path && *(end - 1) == '/')
+		end--;
+
+	/* if a parent directory was found, create it recursively */
+	if (end != path) {
+		*end = '\0';
+		r = mkdir_p(path, mode);
+		*end = '/';
+
+		if (r < 0)
+			return -errno;
+	}
+
+	/* create the desired directory */
+	return mkdir(path, mode);
+}
+
+int mkdir_parents(const char *path, mode_t mode) {
+	char *dirname = strdupa(path);
+	char *offset = strrchr(dirname, '/');
+
+	/* no parent directories */
+	if (!offset) {
+		return 0;
+	}
+
+	/* compute the dirname */
+	while (*(offset - 1) == '/')
+		offset--;
+	*offset = '\0';
+
+	return mkdir_p(dirname, mode);
+}
diff --git a/tools/mkdir.h b/tools/mkdir.h
new file mode 100644
index 0000000..5327fbe
--- /dev/null
+++ b/tools/mkdir.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2012  Lucas De Marchi <lucas.de.marchi@gmail.com>
+ * Copyright (C) 2013  Tom Gundersen <teg@jklm.no>
+ *
+ * This program 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.
+ *
+ * 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#pragma once
+
+#include <sys/stat.h>
+#include <sys/types.h>
+
+int mkdir_parents(const char *path, mode_t mode);
diff --git a/tools/static-nodes.c b/tools/static-nodes.c
index b4a3c08..fb2d633 100644
--- a/tools/static-nodes.c
+++ b/tools/static-nodes.c
@@ -32,6 +32,8 @@
 #include <sys/types.h>
 #include "libkmod-util.h"
 
+#include "mkdir.h"
+
 #include "kmod.h"
 
 struct static_nodes_format {
@@ -230,9 +232,19 @@ static int do_static_nodes(int argc, char *argv[])
 	}
 
 	if (out == NULL) {
+		int r;
+
+		r = mkdir_parents(output, 0755);
+		if (r < 0) {
+			fprintf(stderr, "Error: could not create parent directory for %s - %m.\n",
+				output);
+			ret = EXIT_FAILURE;
+			goto finish;
+		}
+
 		out = fopen(output, "we");
 		if (out == NULL) {
-			fprintf(stderr, "Error: could not create %s!\n",
+			fprintf(stderr, "Error: could not create %s - %m\n",
 				output);
 			ret = EXIT_FAILURE;
 			goto finish;
-- 
1.8.3.2

Re: [PATCH 1/2] static-nodes: don't fail if modules.devname not found

[Dave Reisner]

On Sun, Jul 14, 2013 at 01:56:17PM +0200, Tom Gundersen wrote:
> In containers/VM's/initrd one might not have installed any modules and accompanying modules.devname
> Don't fail if this is the case, just warn.
> 
> When used in systemd this means we don't get a failing unit on booting containers.
> ---
>  tools/static-nodes.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/static-nodes.c b/tools/static-nodes.c
> index 96bb71e..b4a3c08 100644
> --- a/tools/static-nodes.c
> +++ b/tools/static-nodes.c
> @@ -155,6 +155,7 @@ static int do_static_nodes(int argc, char *argv[])
>  {
>  	struct utsname kernel;
>  	char modules[PATH_MAX];
> +	char output[PATH_MAX];
>  	FILE *in = NULL, *out = stdout;
>  	const struct static_nodes_format *format = &static_nodes_format_human;
>  	char buf[4096];
> @@ -170,13 +171,8 @@ static int do_static_nodes(int argc, char *argv[])
>  		}
>  		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;
> -			}
> +			strncpy(output, optarg, PATH_MAX);

No need to copy anything here, just retain a pointer to this optarg.

> +			out = NULL;
>  			break;
>  		case 'f':
>  			valid = 0;
> @@ -221,12 +217,28 @@ static int do_static_nodes(int argc, char *argv[])
>  		 kernel.release);
>  	in = fopen(modules, "re");
>  	if (in == NULL) {
> -		fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname - %m\n",
> -			kernel.release);
> -		ret = EXIT_FAILURE;
> +		if (errno == ENOENT) {
> +			fprintf(stderr, "Warning: /lib/modules/%s/modules.devname not found - ignoring\n",
> +				kernel.release);
> +			ret = EXIT_SUCCESS;
> +		} else {
> +			fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname - %m\n",
> +				kernel.release);
> +			ret = EXIT_FAILURE;
> +		}
>  		goto finish;
>  	}
>  
> +	if (out == NULL) {

It took me a minute to realize why this was the correct comparison.
Maybe it's just me, but I think this would be more readable if we did
something like the below psuedocode

  const char* output = "/dev/stderr";
  /* do option parsing, maybe reassigning 'output' */

  /* now open the file, regardless of what it is */
  FILE* out = fopen(output, "we");
  if (out == NULL)
    ...

Seems a bit cleaner to me, even if it duplicates a file descriptor (but
we'll open a new FILE regardless in the common use case).

> +		out = fopen(output, "we");
> +		if (out == NULL) {
> +			fprintf(stderr, "Error: could not create %s!\n",
> +				output);

I realize this is copied from the old code, but there's no explanation
as to why this failed. Add an %m token to the format string?

> +			ret = EXIT_FAILURE;
> +			goto finish;
> +		}
> +	}
> +
>  	while (fgets(buf, sizeof(buf), in) != NULL) {
>  		char modname[PATH_MAX];
>  		char devname[PATH_MAX];
> -- 
> 1.8.3.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-modules" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] static-nodes: don't fail if modules.devname not found

[Tom Gundersen]

On Sun, Jul 14, 2013 at 2:29 PM, Dave Reisner <d@falconindy.com> wrote:
> On Sun, Jul 14, 2013 at 01:56:17PM +0200, Tom Gundersen wrote:
>> +                     strncpy(output, optarg, PATH_MAX);
>
> No need to copy anything here, just retain a pointer to this optarg.

Fixed.

>> +     if (out == NULL) {
>
> It took me a minute to realize why this was the correct comparison.
> Maybe it's just me, but I think this would be more readable if we did
> something like the below psuedocode
>
>   const char* output = "/dev/stderr";
>   /* do option parsing, maybe reassigning 'output' */
>
>   /* now open the file, regardless of what it is */
>   FILE* out = fopen(output, "we");
>   if (out == NULL)
>     ...
>
> Seems a bit cleaner to me, even if it duplicates a file descriptor (but
> we'll open a new FILE regardless in the common use case).

Yeah, makes sense to me, I'll do that instead.

>> +             out = fopen(output, "we");
>> +             if (out == NULL) {
>> +                     fprintf(stderr, "Error: could not create %s!\n",
>> +                             output);
>
> I realize this is copied from the old code, but there's no explanation
> as to why this failed. Add an %m token to the format string?

For some reason that change ended up in the subsequent patch, anyway,
moving it to this one where it belong.

Thanks.

Tom

Re: [PATCH 1/2] static-nodes: don't fail if modules.devname not found

[Kay Sievers]

On Sun, Jul 14, 2013 at 1:56 PM, Tom Gundersen <teg@jklm.no> wrote:

> @@ -170,13 +171,8 @@ static int do_static_nodes(int argc, char *argv[])
>                 }
>                 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;
> -                       }
> +                       strncpy(output, optarg, PATH_MAX);

Not relevant for any common use case, but strncpy() sucks and will not
terminate with '\0', so we should probably just always add a '\0' to
the end.

Kay