Libguestfs - Apr 2017 - [PATCH] appliance: Pass root=UUID=... to supermin.

By passing root=UUID=... to supermin, we make the appliance boot
process less sensitive to the non-deterministic process of scanning
SCSI disks (of which much more to come).

This patch should be tested alongside the supermin patch posted here:

  https://www.redhat.com/archives/libguestfs/2017-April/msg00174.html

which in turn requires this supermin patch series:

  https://www.redhat.com/archives/libguestfs/2017-April/msg00162.html

This will require that libguestfs & supermin are upgraded in step in
all downstream distributions, and will also invalidate all binary
appliances (although we don't ship a binary appliance for 1.37 so
that's not a problem).

Rich.

Instead of mounting the appliance by device name (eg. root=/dev/sdb),
mount the appliance filesystem by filesystem UUID.  This allows the
appliance to be robust against the non-determinism of SCSI device
enumeration, but it does require a corresponding change to be added to
supermin, and therefore a new version of supermin.

I did not make a corresponding change to UML as it is unlikely to have
a problem with enumerating ubd devices.
---
 docs/guestfs-building.pod |  2 +-
 lib/Makefile.am           |  1 +
 lib/appliance-kcmdline.c  | 10 ++---
 lib/appliance-uuid.c      | 95 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/guestfs-internal.h    |  3 ++
 lib/launch-direct.c       | 36 +++---------------
 lib/launch-libvirt.c      | 19 +++++++---
 m4/guestfs_appliance.m4   |  2 +-
 8 files changed, 124 insertions(+), 44 deletions(-)
 create mode 100644 lib/appliance-uuid.c

diff --git a/docs/guestfs-building.pod b/docs/guestfs-building.pod
index bfb46a02f..775d590f3 100644
--- a/docs/guestfs-building.pod
+++ b/docs/guestfs-building.pod
@@ -80,7 +80,7 @@ I<Required>.  Virt-p2v and virt-v2v requires qemu-img
E<ge> 2.2.0.
 I<Required>.  The following features must be enabled:
 C<virtio-pci>, C<virtio-serial>, C<virtio-block>,
C<virtio-net>.
 
-=item supermin E<ge> 5.1.0
+=item supermin E<ge> 5.1.18
 
 I<Required>.  For alternatives, see L</USING A PREBUILT BINARY
APPLIANCE>
 below.
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 063706f8f..01f499e6d 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -75,6 +75,7 @@ libguestfs_la_SOURCES = \
 	appliance-cpu.c \
 	appliance-kcmdline.c \
 	appliance-uefi.c \
+	appliance-uuid.c \
 	available.c \
 	bindtests.c \
 	canonical-name.c \
diff --git a/lib/appliance-kcmdline.c b/lib/appliance-kcmdline.c
index 4dde7a865..efb21e829 100644
--- a/lib/appliance-kcmdline.c
+++ b/lib/appliance-kcmdline.c
@@ -58,9 +58,7 @@
  * located in this file because it's a convenient place for this
  * common code.
  *
- * The C<appliance_dev> parameter must be the full device name of the
- * appliance disk and must have already been adjusted to take into
- * account virtio-blk or virtio-scsi; eg C</dev/sdb>.
+ * The C<appliance_uuid> is the UUID of the appliance filesystem.
  *
  * The C<flags> parameter can contain the following flags logically
  * or'd together (or 0):
@@ -78,7 +76,7 @@
  * be freed by the caller.
  */
 char *
-guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_dev,
+guestfs_int_appliance_command_line (guestfs_h *g, const char *appliance_uuid,
 				    int flags)
 {
   CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (argv);
@@ -162,8 +160,8 @@ guestfs_int_appliance_command_line (guestfs_h *g, const char
*appliance_dev,
   guestfs_int_add_string (g, &argv, "8250.nr_uarts=1");
 
   /* Tell supermin about the appliance device. */
-  if (appliance_dev)
-    guestfs_int_add_sprintf (g, &argv, "root=%s", appliance_dev);
+  if (appliance_uuid)
+    guestfs_int_add_sprintf (g, &argv, "root=UUID=%s",
appliance_uuid);
 
   /* SELinux - deprecated setting, never worked and should not be enabled. */
   if (g->selinux)
diff --git a/lib/appliance-uuid.c b/lib/appliance-uuid.c
new file mode 100644
index 000000000..80dc319ae
--- /dev/null
+++ b/lib/appliance-uuid.c
@@ -0,0 +1,95 @@
+/* libguestfs
+ * Copyright (C) 2010-2017 Red Hat Inc.
+ *
+ * This library 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 of the License, or (at your option) any later version.
+ *
+ * This library 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 Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * Calculate the root=UUID=... kernel parameter for the appliance.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+/**
+ * Calculate the volume UUID of an ext4 filesystem (in a file).  This
+ * is quite simple as it is stored at a known offset.  See
+ *
L<https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout#The_Super_Block>.
+ */
+char *
+guestfs_int_ext4fs_uuid (guestfs_h *g, const char *filename)
+{
+  int fd;
+  unsigned char magic[2];
+  unsigned char raw_uuid[16];
+  char uuid[37];
+
+  fd = open (filename, O_RDONLY);
+  if (fd == -1) {
+    perrorf (g, "%s", filename);
+    return NULL;
+  }
+
+  /* Check it's really an ext4 superblock at offset 0x400. */
+  if (pread (fd, magic, sizeof magic, 0x438) != sizeof magic) {
+    error (g, "ext4fs: could not read magic from ext4 file: %s",
filename);
+    close (fd);
+    return NULL;
+  }
+  if (magic[0] != 0x53 || magic[1] != 0xef) {
+    error (g, "ext4fs: not an ext4 file: %s", filename);
+    close (fd);
+    return NULL;
+  }
+
+  /* Read the raw bytes of the UUID (128 bits / 8 == 16 bytes). */
+  if (pread (fd, raw_uuid, sizeof raw_uuid, 0x468) != sizeof raw_uuid) {
+    error (g, "ext4fs: could not read volume UUID from ext4 superblock:
%s",
+           filename);
+    close (fd);
+    return NULL;
+  }
+  close (fd);
+
+  /* Check for sanity. */
+  if (is_zero ((char *) raw_uuid, sizeof raw_uuid)) {
+    error (g, "ext4fs: UUID is all zeroes: %s", filename);
+    return NULL;
+  }
+
+  /* Convert to a string UUID.  The same format that blkid produces. */
+  snprintf (uuid, sizeof uuid,
+            "%02x%02x%02x%02x-"
+            "%02x%02x-"
+            "%02x%02x-"
+            "%02x%02x-"
+            "%02x%02x%02x%02x%02x%02x",
+            raw_uuid[0], raw_uuid[1], raw_uuid[2], raw_uuid[3],
+            raw_uuid[4], raw_uuid[5],
+            raw_uuid[6], raw_uuid[7],
+            raw_uuid[8], raw_uuid[9],
+            raw_uuid[10], raw_uuid[11], raw_uuid[12], raw_uuid[13],
+            raw_uuid[14], raw_uuid[15]);
+
+  return safe_strdup (g, uuid); /* caller frees */
+}
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index a04ccff09..c181396ec 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -847,6 +847,9 @@ extern char *guestfs_int_appliance_command_line (guestfs_h
*g, const char *appli
 /* appliance-uefi.c */
 extern int guestfs_int_get_uefi (guestfs_h *g, char **code, char **vars, int
*flags);
 
+/* appliance-uuid.c */
+extern char *guestfs_int_ext4fs_uuid (guestfs_h *g, const char *filename);
+
 /* launch.c */
 extern int64_t guestfs_int_timeval_diff (const struct timeval *x, const struct
timeval *y);
 extern void guestfs_int_launch_send_progress (guestfs_h *g, int perdozen);
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index b0038c6a9..30ed6b1f9 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -60,7 +60,6 @@ struct backend_direct_data {
 };
 
 static int is_openable (guestfs_h *g, const char *path, int flags);
-static char *make_appliance_dev (guestfs_h *g);
 static void print_qemu_command_line (guestfs_h *g, char **argv);
 
 static char *
@@ -229,7 +228,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   int uefi_flags;
   CLEANUP_FREE char *kernel = NULL, *initrd = NULL, *appliance = NULL;
   int has_appliance_drive;
-  CLEANUP_FREE char *appliance_dev = NULL;
+  CLEANUP_FREE char *appliance_uuid = NULL;
   uint32_t size;
   CLEANUP_FREE void *buf = NULL;
   struct drive *drv;
@@ -555,7 +554,9 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
     ADD_CMDLINE ("-device");
     ADD_CMDLINE ("scsi-hd,drive=appliance");
 
-    appliance_dev = make_appliance_dev (g);
+    appliance_uuid = guestfs_int_ext4fs_uuid (g, appliance);
+    if (!appliance_uuid)
+      goto cleanup0;
   }
 
   /* Create the virtio serial bus. */
@@ -598,7 +599,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   if (!has_kvm || force_tcg)
     flags |= APPLIANCE_COMMAND_LINE_IS_TCG;
   ADD_CMDLINE_STRING_NODUP
-    (guestfs_int_appliance_command_line (g, appliance_dev, flags));
+    (guestfs_int_appliance_command_line (g, appliance_uuid, flags));
 
   /* Note: custom command line parameters must come last so that
    * qemu -set parameters can modify previously added options.
@@ -852,33 +853,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   return -1;
 }
 
-/* Calculate the appliance device name.
- *
- * The easy thing would be to use g->nr_drives (indeed, that's what we
- * used to do).  However this breaks if some of the drives being added
- * use the deprecated 'iface' parameter.  To further add confusion,
- * the format of the 'iface' parameter has never been defined, but
- * given existing usage we can assume it has one of only three values:
- * NULL, "ide" or "virtio" (which means virtio-blk).  See
RHBZ#975797.
- */
-static char *
-make_appliance_dev (guestfs_h *g)
-{
-  size_t i, index = 0;
-  struct drive *drv;
-  char dev[64] = "/dev/sd";
-
-  /* Calculate the index of the drive. */
-  ITER_DRIVES (g, i, drv) {
-    if (drv->iface == NULL || STREQ (drv->iface, "ide"))
-      index++;
-  }
-
-  guestfs_int_drive_name (index, &dev[7]);
-
-  return safe_strdup (g, dev);  /* Caller frees. */
-}
-
 /* This is called from the forked subprocess just before qemu runs, so
  * it can just print the message straight to stderr, where it will be
  * picked up and funnelled through the usual appliance event API.
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index f66c8e0ef..8a95cbbe5 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -136,7 +136,7 @@ struct libvirt_xml_params {
   char *kernel;                 /* paths to kernel and initrd */
   char *initrd;
   char *appliance_overlay;      /* path to qcow2 overlay backed by appliance */
-  char appliance_dev[64];       /* appliance device name */
+  char *appliance_uuid;         /* appliance volume UUID */
   size_t appliance_index;       /* index of appliance */
   bool enable_svirt;            /* false if we decided to disable sVirt */
   bool current_proc_is_root;    /* true = euid is root */
@@ -303,6 +303,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
     .kernel = NULL,
     .initrd = NULL,
     .appliance_overlay = NULL,
+    .appliance_uuid = NULL,
   };
   CLEANUP_FREE xmlChar *xml = NULL;
   CLEANUP_FREE char *appliance = NULL;
@@ -582,8 +583,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   debug (g, "create libvirt XML");
 
   params.appliance_index = g->nr_drives;
-  strcpy (params.appliance_dev, "/dev/sd");
-  guestfs_int_drive_name (params.appliance_index,
&params.appliance_dev[7]);
+  params.appliance_uuid = guestfs_int_ext4fs_uuid (g, appliance);
+  if (!params.appliance_uuid)
+    goto cleanup;
   params.enable_svirt = ! is_custom_hv (g);
 
   xml = construct_libvirt_xml (g, &params);
@@ -690,6 +692,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   free (params.kernel);
   free (params.initrd);
   free (params.appliance_overlay);
+  free (params.appliance_uuid);
 
   return 0;
 
@@ -715,6 +718,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   free (params.kernel);
   free (params.initrd);
   free (params.appliance_overlay);
+  free (params.appliance_uuid);
 
   g->state = CONFIG;
 
@@ -1202,7 +1206,8 @@ construct_libvirt_xml_boot (guestfs_h *g,
   flags = 0;
   if (!params->data->is_kvm)
     flags |= APPLIANCE_COMMAND_LINE_IS_TCG;
-  cmdline = guestfs_int_appliance_command_line (g, params->appliance_dev,
flags);
+  cmdline = guestfs_int_appliance_command_line (g, params->appliance_uuid,
+                                                flags);
 
   start_element ("os") {
     start_element ("type") {
@@ -1743,6 +1748,10 @@ construct_libvirt_xml_appliance (guestfs_h *g,
                                  const struct libvirt_xml_params *params,
                                  xmlTextWriterPtr xo)
 {
+  char dev[64] = "sd";
+
+  guestfs_int_drive_name (params->appliance_index, &dev[2]);
+
   start_element ("disk") {
     attribute ("type", "file");
     attribute ("device", "disk");
@@ -1752,7 +1761,7 @@ construct_libvirt_xml_appliance (guestfs_h *g,
     } end_element ();
 
     start_element ("target") {
-      attribute ("dev", &params->appliance_dev[5]);
+      attribute ("dev", dev);
       attribute ("bus", "scsi");
     } end_element ();
 
diff --git a/m4/guestfs_appliance.m4 b/m4/guestfs_appliance.m4
index 890b1999c..20a0957cc 100644
--- a/m4/guestfs_appliance.m4
+++ b/m4/guestfs_appliance.m4
@@ -34,7 +34,7 @@ then you have to --disable-appliance as well, since the
appliance contains
 the daemon inside it.])
 fi
 
-dnl Check for supermin >= 5.1.0.
+dnl Check for supermin >= 5.1.18.
 AC_PATH_PROG([SUPERMIN],[supermin],[no])
 
 dnl Pass supermin --packager-config option.
-- 
2.12.0