Libguestfs - Apr 2017 - Re: [PATCH supermin 3/3] init: Refactor for-loop which waits for root device to show up.

Require the root= parameter is passed to specify which
root (apppliance) to mount.  Libguestfs has done this since 2012.

The other two patches are small code refactorings in the init
program.

Rich.

In old versions of supermin, we picked the last device as the root
device.  Since 2012, libguestfs has passed the correct device to use
in the root= parameter, and this was supported by supermin (actually,
febootstrap!) >= 3.15.

This change requires the root= parameter to be passed, and supermin
will fail with an error otherwise.
---
 init/init.c | 97 +++++++++++++++++++++++--------------------------------------
 1 file changed, 37 insertions(+), 60 deletions(-)

diff --git a/init/init.c b/init/init.c
index 46325b4..e3d1107 100644
--- a/init/init.c
+++ b/init/init.c
@@ -154,76 +154,53 @@ main ()
   }
   fclose (fp);
 
-  /* Look for the ext2 filesystem device.  It's always the last one
-   * that was added.  Modern versions of libguestfs supply the
-   * expected name of the root device on the command line
-   * ("root=/dev/...").  For virtio-scsi this is required, because we
-   * must wait for the device to appear after the module is loaded.
+  /* Look for the ext2 filesystem root device specified as root=...
+   * on the kernel command line.
    */
   char *root, *path;
   size_t len;
   int dax = 0;
   root = strstr (cmdline, "root=");
-  if (root) {
-    root += 5;
-    if (strncmp (root, "/dev/", 5) == 0)
-      root += 5;
-    if (strncmp (root, "pmem", 4) == 0)
-      dax = 1;
-    len = strcspn (root, " ");
-    root[len] = '\0';
-
-    asprintf (&path, "/sys/block/%s/dev", root);
-
-    uint64_t delay_ns = 250000;
-    int virtio_message = 0;
-    while (delay_ns <= MAX_ROOT_WAIT * UINT64_C(1000000000)) {
-      fp = fopen (path, "r");
-      if (fp != NULL)
-        goto found;
-
-      if (delay_ns > 1000000000) {
-	fprintf (stderr,
-		 "supermin: waiting another %" PRIu64 " ns for %s to
appear\n",
-		 delay_ns, path);
-        if (!virtio_message) {
-          fprintf (stderr,
-                   "This usually means your kernel doesn't support
virtio, or supermin was unable\n"
-                   "to load some kernel modules (see module loading
messages above).\n");
-          virtio_message = 1;
-        }
-      }
-
-      struct timespec t;
-      t.tv_sec = delay_ns / 1000000000;
-      t.tv_nsec = delay_ns % 1000000000;
-      nanosleep (&t, NULL);
-      delay_ns *= 2;
-    }
+  if (!root) {
+    fprintf (stderr, "supermin: missing root= parameter on the command
line\n");
+    exit (EXIT_FAILURE);
   }
-  else {
-    path = strdup ("/sys/block/xdx/dev");
-
-    char class[3] = { 'v', 's', 'h' };
-    size_t i, j;
-    fp = NULL;
-    for (i = 0; i < sizeof class; ++i) {
-      for (j = 'z'; j >= 'a'; --j) {
-        path[11] = class[i];
-        path[13] = j;
-        fp = fopen (path, "r");
-        if (fp != NULL)
-          goto found;
+  root += 5;
+  if (strncmp (root, "/dev/", 5) == 0)
+    root += 5;
+  if (strncmp (root, "pmem", 4) == 0)
+    dax = 1;
+  len = strcspn (root, " ");
+  root[len] = '\0';
+
+  asprintf (&path, "/sys/block/%s/dev", root);
+
+  uint64_t delay_ns = 250000;
+  int virtio_message = 0;
+  while (delay_ns <= MAX_ROOT_WAIT * UINT64_C(1000000000)) {
+    fp = fopen (path, "r");
+    if (fp != NULL)
+      break;
+
+    if (delay_ns > 1000000000) {
+      fprintf (stderr,
+               "supermin: waiting another %" PRIu64 " ns for %s
to appear\n",
+               delay_ns, path);
+      if (!virtio_message) {
+        fprintf (stderr,
+                 "This usually means your kernel doesn't support
virtio, or supermin was unable\n"
+                 "to load some kernel modules (see module loading messages
above).\n");
+        virtio_message = 1;
       }
     }
-  }
 
-  fprintf (stderr,
-           "supermin: no ext2 root device found\n"
-           "Please include FULL verbose output in your bug
report.\n");
-  exit (EXIT_FAILURE);
+    struct timespec t;
+    t.tv_sec = delay_ns / 1000000000;
+    t.tv_nsec = delay_ns % 1000000000;
+    nanosleep (&t, NULL);
+    delay_ns *= 2;
+  }
 
- found:
   if (!quiet)
     fprintf (stderr, "supermin: picked %s as root device\n", path);
 
-- 
2.12.0

No functional change, just change the style to the one used
by libguestfs.
---
 init/init.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/init/init.c b/init/init.c
index e3d1107..473a5c5 100644
--- a/init/init.c
+++ b/init/init.c
@@ -96,6 +96,18 @@ static char line[1024];
 int
 main ()
 {
+  FILE *fp;
+  size_t n;
+  char *root, *path;
+  size_t len;
+  int dax = 0;
+  uint64_t delay_ns = 250000;
+  int virtio_message = 0;
+  struct timespec t;
+  int major, minor;
+  char *p;
+  const char *mount_options = "";
+
   mount_proc ();
 
   fprintf (stderr, "supermin: ext2 mini initrd starting up: "
@@ -132,13 +144,13 @@ main ()
     exit (EXIT_FAILURE);
   }
 
-  FILE *fp = fopen ("/modules", "r");
+  fp = fopen ("/modules", "r");
   if (fp == NULL) {
     perror ("fopen: /modules");
     exit (EXIT_FAILURE);
   }
   while (fgets (line, sizeof line, fp)) {
-    size_t n = strlen (line);
+    n = strlen (line);
     if (n > 0 && line[n-1] == '\n')
       line[--n] = '\0';
 
@@ -157,9 +169,6 @@ main ()
   /* Look for the ext2 filesystem root device specified as root=...
    * on the kernel command line.
    */
-  char *root, *path;
-  size_t len;
-  int dax = 0;
   root = strstr (cmdline, "root=");
   if (!root) {
     fprintf (stderr, "supermin: missing root= parameter on the command
line\n");
@@ -175,8 +184,6 @@ main ()
 
   asprintf (&path, "/sys/block/%s/dev", root);
 
-  uint64_t delay_ns = 250000;
-  int virtio_message = 0;
   while (delay_ns <= MAX_ROOT_WAIT * UINT64_C(1000000000)) {
     fp = fopen (path, "r");
     if (fp != NULL)
@@ -194,7 +201,6 @@ main ()
       }
     }
 
-    struct timespec t;
     t.tv_sec = delay_ns / 1000000000;
     t.tv_nsec = delay_ns % 1000000000;
     nanosleep (&t, NULL);
@@ -205,9 +211,9 @@ main ()
     fprintf (stderr, "supermin: picked %s as root device\n", path);
 
   fgets (line, sizeof line, fp);
-  int major = atoi (line);
-  char *p = line + strcspn (line, ":") + 1;
-  int minor = atoi (p);
+  major = atoi (line);
+  p = line + strcspn (line, ":") + 1;
+  minor = atoi (p);
 
   fclose (fp);
   if (umount ("/sys") == -1) {
@@ -225,7 +231,7 @@ main ()
   }
 
   /* Construct the filesystem mount options. */
-  const char *mount_options = "";
+  mount_options = "";
   if (dax)
     mount_options = "dax";
 
@@ -280,27 +286,30 @@ static void
 insmod (const char *filename)
 {
   size_t size;
+  int fd;
+  struct stat st;
+  char *buf;
+  size_t offset;
 
   if (!quiet)
     fprintf (stderr, "supermin: internal insmod %s\n", filename);
 
-  int fd = open (filename, O_RDONLY);
+  fd = open (filename, O_RDONLY);
   if (fd == -1) {
     fprintf (stderr, "insmod: open: %s: %m\n", filename);
     exit (EXIT_FAILURE);
   }
-  struct stat st;
   if (fstat (fd, &st) == -1) {
     perror ("insmod: fstat");
     exit (EXIT_FAILURE);
   }
   size = st.st_size;
-  char *buf = malloc (size);
+  buf = malloc (size);
   if (buf == NULL) {
     fprintf (stderr, "insmod: malloc (%s, %zu bytes): %m\n",
filename, size);
     exit (EXIT_FAILURE);
   }
-  size_t offset = 0;
+  offset = 0;
   do {
     ssize_t rc = read (fd, buf + offset, size - offset);
     if (rc == -1) {
-- 
2.12.0

---
 init/init.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/init/init.c b/init/init.c
index 473a5c5..a6279b8 100644
--- a/init/init.c
+++ b/init/init.c
@@ -101,13 +101,19 @@ main ()
   char *root, *path;
   size_t len;
   int dax = 0;
-  uint64_t delay_ns = 250000;
+  uint64_t delay_ns;
   int virtio_message = 0;
-  struct timespec t;
   int major, minor;
   char *p;
   const char *mount_options = "";
 
+  struct timespec t;
+#define NANOSLEEP(ns) do {                      \
+    t.tv_sec = delay_ns / 1000000000;           \
+    t.tv_nsec = delay_ns % 1000000000;          \
+    nanosleep (&t, NULL);                       \
+  } while(0)
+
   mount_proc ();
 
   fprintf (stderr, "supermin: ext2 mini initrd starting up: "
@@ -184,7 +190,9 @@ main ()
 
   asprintf (&path, "/sys/block/%s/dev", root);
 
-  while (delay_ns <= MAX_ROOT_WAIT * UINT64_C(1000000000)) {
+  for (delay_ns = 250000;
+       delay_ns <= MAX_ROOT_WAIT * UINT64_C(1000000000);
+       delay_ns *= 2) {
     fp = fopen (path, "r");
     if (fp != NULL)
       break;
@@ -201,10 +209,7 @@ main ()
       }
     }
 
-    t.tv_sec = delay_ns / 1000000000;
-    t.tv_nsec = delay_ns % 1000000000;
-    nanosleep (&t, NULL);
-    delay_ns *= 2;
+    NANOSLEEP (delay_ns);
   }
 
   if (!quiet)
-- 
2.12.0

On Wed, Apr 19, 2017 at 10:57:28AM +0100, Richard W.M. Jones
wrote:
> +  struct timespec t;
> +#define NANOSLEEP(ns) do {                      \
> +    t.tv_sec = delay_ns / 1000000000;           \
> +    t.tv_nsec = delay_ns % 1000000000;          \
> +    nanosleep (&t, NULL);                       \
> +  } while(0)
> +
Defining 't' outside its context is a bit clumsy.  See attached
version of 3/3 which fixes this.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Wednesday, 19 April 2017 11:57:27 CEST Richard W.M. Jones
wrote:
> No functional change, just change the style to the one used
> by libguestfs.
> ---
Mostly LGTM.
>    while (fgets (line, sizeof line, fp)) {
> -    size_t n = strlen (line);
> +    n = strlen (line);
This one I would leave -- as it avoids using a wrong/nonsense value
outside of the loop, and at the same time making sure it is properly
reset every time.

Thanks,
-- 
Pino Toscano

On Wednesday, 19 April 2017 11:57:25 CEST Richard W.M. Jones
wrote:
> Require the root= parameter is passed to specify which
> root (apppliance) to mount.  Libguestfs has done this since 2012.
> 
> The other two patches are small code refactorings in the init
> program.
LGTM, with notes where noted.

Thanks,
-- 
Pino Toscano