Libguestfs - Mar 2020 - [PATCH v2 0/4] daemon: Translate device names if Linux device is unstable (RHBZ#1804207).

v1 was here:
https://www.redhat.com/archives/libguestfs/2020-February/msg00220.html

This patch series is a little bit better.  It's still a bit of a hack.
The _real_ fix for this is outlined in the TODO file (see patch 1) but
that requires a lot more work than we could do before 1.42 is
released, unless we delay 1.42 for a lot longer.  I'm hoping with this
to have something which works for 1.42, and then fix it properly in
1.43.

Patch 2 is a debugging patch.  It's separated out because we might
consider reverting it in future once we are confident that the
approach is working.

Patch 3 is the second change necessary to how we launch the supermin
appliance.  This actually stands alone so could be reviewed
separately.  It requires a modest update to our baseline supermin
version.

Patch 4 should be ignored, it's just for my (additional) debugging.

Rich.

Linux from around 5.6 now enumerates individual disks in any order
(whereas previously it enumerated only drivers in parallel).  This
means that /dev/sdX ordering is no longer stable - in particular we
cannot be sure that /dev/sda inside the guest is the first disk that
was attached to the appliance, /dev/sdb the second disk and so on.

However we can still use SCSI PCI device numbering as found in
/dev/disk/by-path.  Use this to translate device names in and out of
the appliance.

Thanks: Vitaly Kuznetsov, Paolo Bonzini, Dan Berrangé.
---
 TODO                             |  13 +++
 daemon/daemon.h                  |   2 +
 daemon/device-name-translation.c | 195 +++++++++++++++++++++++++++++--
 daemon/devsparts.ml              |  23 +++-
 daemon/guestfsd.c                |   5 +
 daemon/stubs-macros.h            |  10 +-
 lib/canonical-name.c             |   5 +
 7 files changed, 235 insertions(+), 18 deletions(-)

diff --git a/TODO b/TODO
index 066f633d4..064386ac2 100644
--- a/TODO
+++ b/TODO
@@ -189,6 +189,19 @@ might be replaced by direct use of the library (if this is
easier).
 But it is very hard to be compatible between RHEL6 and RHEL5 when
 using the library directly.
 
+Properly fix device name translation
+------------------------------------
+
+Currently for Device parameters we pass a string name like "/dev/sda"
+or "/dev/sdb2" or "/dev/VG/LV" to the daemon.  Since Linux
broke
+device enumeration (RHBZ#1804207) this works less well, requiring
+non-trivial translation within the daemon.
+
+It would be far better if in the generator we mapped "/dev/XXX" to a
+proper structure.  Device index + partition | LV | MD | ...
+This would be then used everywhere inside the daemon and mean we would
+no longer need device name translation at all.
+
 Visualization
 -------------
 
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 170fb2537..24cf8585d 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -215,6 +215,8 @@ extern void notify_progress_no_ratelimit (uint64_t position,
uint64_t total, con
 
 /* device-name-translation.c */
 extern char *device_name_translation (const char *device);
+extern void device_name_translation_init (void);
+extern void device_name_translation_free (void);
 extern char *reverse_device_name_translation (const char *device);
 
 /* stubs.c (auto-generated) */
diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c
index ed826bbae..30173f5c2 100644
--- a/daemon/device-name-translation.c
+++ b/daemon/device-name-translation.c
@@ -27,12 +27,125 @@
 #include <dirent.h>
 #include <limits.h>
 #include <sys/stat.h>
+#include <errno.h>
+#include <error.h>
+
+#include "c-ctype.h"
 
 #include "daemon.h"
 
+static char **cache;
+static size_t cache_size;
+
+/**
+ * Cache daemon disk mapping.
+ *
+ * When the daemon starts up, populate a cache with the contents
+ * of /dev/disk/by-path.  It's easiest to use C<ls -lv> here
+ * since the names are sorted awkwardly.
+ */
+void
+device_name_translation_init (void)
+{
+  const char *by_path = "/dev/disk/by-path";
+  CLEANUP_FREE char *out = NULL, *err = NULL;
+  CLEANUP_FREE_STRING_LIST char **lines = NULL;
+  size_t i, j;
+  int r;
+
+  device_name_translation_free ();
+
+  r = command (&out, &err, "ls", "-1v", by_path,
NULL);
+  if (r == -1)
+    error (EXIT_FAILURE, 0,
+           "failed to initialize device name translation cache: %s",
err);
+
+  lines = split_lines (out);
+  if (lines == NULL)
+    error (EXIT_FAILURE, errno, "split_lines");
+
+  cache_size = guestfs_int_count_strings (lines);
+  cache = calloc (cache_size, sizeof (char *));
+  if (cache == NULL)
+    error (EXIT_FAILURE, errno, "calloc");
+
+  /* Delete entries for partitions.  (Mark them as NULL in the array
+   * and skip them below.)
+   */
+  for (i = 0; i < cache_size; ++i) {
+    if (strstr (lines[i], "-part"))
+      lines[i] = NULL;
+  }
+
+  /* Look up each device name.  It should be a symlink to /dev/sdX. */
+  for (i = j = 0; i < cache_size; ++i) {
+    if (lines[i] != NULL) {
+      CLEANUP_FREE char *full;
+      char *device;
+
+      if (asprintf (&full, "%s/%s", by_path, lines[i]) == -1)
+        error (EXIT_FAILURE, errno, "asprintf");
+
+      device = realpath (full, NULL);
+      if (device == NULL)
+        error (EXIT_FAILURE, errno, "realpath: %s", full);
+
+      /* Don't add the root device to the cache. */
+      if (is_root_device (device))
+        continue;
+
+      cache[j++] = device;
+    }
+  }
+
+  /* This is the final cache size because we deleted entries above. */
+  cache_size = j;
+}
+
+void
+device_name_translation_free (void)
+{
+  size_t i;
+
+  for (i = 0; i < cache_size; ++i)
+    free (cache[i]);
+  free (cache);
+  cache = NULL;
+  cache_size = 0;
+}
+
 /**
  * Perform device name translation.
  *
+ * Libguestfs defines a few standard formats for device names.
+ * (see also L<guestfs(3)/BLOCK DEVICE NAMING> and
+ * L<guestfs(3)/guestfs_canonical_device_name>).  They are:
+ *
+ * =over 4
+ *
+ * =item F</dev/sdX[N]>
+ *
+ * =item F</dev/hdX[N]>
+ *
+ * =item F</dev/vdX[N]>
+ *
+ * These mean the Nth partition on the Xth device.  Because
+ * Linux no longer enumerates devices in the order they are
+ * passed to qemu, we must translate these by looking up
+ * the actual device using /dev/disk/by-path/
+ *
+ * =item F</dev/mdX>
+ *
+ * =item F</dev/VG/LV>
+ *
+ * =item F</dev/mapper/...>
+ *
+ * =item F</dev/dm-N>
+ *
+ * These are not translated here.
+ *
+ * =back
+ *
  * It returns a newly allocated string which the caller must free.
  *
  * It returns C<NULL> on error.  B<Note> it does I<not> call
@@ -45,18 +158,59 @@ char *
 device_name_translation (const char *device)
 {
   int fd;
-  char *ret;
+  char *ret = NULL;
+  size_t len;
 
-  fd = open (device, O_RDONLY|O_CLOEXEC);
+  /* /dev/sdX[N] and aliases like /dev/vdX[N]. */
+  if (STRPREFIX (device, "/dev/") &&
+      strchr (device+5, '/') == NULL && /* not an LV name */
+      device[5] != 'm' && /* not /dev/md - RHBZ#1414682 */
+      ((len = strcspn (device+5, "d")) > 0 && len <=
2)) {
+    ssize_t i;
+    const char *start;
+    char dev[16];
+
+    /* Translate to a disk index in /dev/disk/by-path sorted numerically. */
+    start = &device[5+len+1];
+    len = strspn (start, "abcdefghijklmnopqrstuvwxyz");
+    if (len >= sizeof dev - 1) {
+      fprintf (stderr, "unparseable device name: %s\n", device);
+      return NULL;
+    }
+    strcpy (dev, start);
+    dev[len] = '\0';
+
+    i = guestfs_int_drive_index (dev);
+    if (i >= 0 && i < (ssize_t) cache_size) {
+      /* Append the partition name if present. */
+      if (asprintf (&ret, "%s%s", cache[i], start+len) == -1)
+        return NULL;
+    }
+  }
+
+  /* If we didn't translate it above, continue with the same name. */
+  if (ret == NULL) {
+    ret = strdup (device);
+    if (ret == NULL)
+      return NULL;
+  }
+
+  /* Now check the device is openable. */
+  fd = open (ret, O_RDONLY|O_CLOEXEC);
   if (fd >= 0) {
     close (fd);
-    return strdup (device);
+    return ret;
   }
 
-  if (errno != ENXIO && errno != ENOENT)
+  if (errno != ENXIO && errno != ENOENT) {
+    perror (ret);
+    free (ret);
     return NULL;
+  }
 
-  /* If the name begins with "/dev/sd" then try the alternatives. */
+  free (ret);
+
+  /* If the original name begins with "/dev/sd" then try the
alternatives. */
   if (!STRPREFIX (device, "/dev/sd"))
     return NULL;
   device += 7;                  /* device == "a1" etc. */
@@ -97,13 +251,34 @@ device_name_translation (const char *device)
 char *
 reverse_device_name_translation (const char *device)
 {
-  char *ret;
+  char *ret = NULL;
+  size_t i;
+
+  /* Look it up in the cache, and if found return the canonical name.
+   * If not found return a copy of the original string.
+   */
+  for (i = 0; i < cache_size; ++i) {
+    const size_t len = strlen (cache[i]);
+
+    if (STREQ (device, cache[i]) ||
+        (STRPREFIX (device, cache[i]) && c_isdigit (device[len]))) {
+      char drv[16];
+
+      guestfs_int_drive_name (i, drv);
+      if (asprintf (&ret, "/dev/sd%s%s", drv, &device[len])
== -1) {
+        reply_with_perror ("asprintf");
+        return NULL;
+      }
+      break;
+    }
+  }
 
-  /* Currently a no-op. */
-  ret = strdup (device);
   if (ret == NULL) {
-    reply_with_perror ("strdup");
-    return NULL;
+    ret = strdup (device);
+    if (ret == NULL) {
+      reply_with_perror ("strdup");
+      return NULL;
+    }
   }
 
   return ret;
diff --git a/daemon/devsparts.ml b/daemon/devsparts.ml
index 59e66e82e..b0703d3ab 100644
--- a/daemon/devsparts.ml
+++ b/daemon/devsparts.ml
@@ -23,9 +23,26 @@ open Std_utils
 
 open Utils
 
-let map_block_devices ~return_md f -  let devs = Sys.readdir
"/sys/block" in
-  let devs = Array.to_list devs in
+let rec map_block_devices ~return_md f +  (* We need to get the devices in
translated order.  Use the
+   * same command that we use in device-name-translation.c.
+   *)
+  let path = "/dev/disk/by-path" in
+  let devs = command "ls" ["-1v"; path] in
+  let devs = String.trimr devs in
+  let devs = String.nsplit "\n" devs in
+  (* Delete entries for partitions. *)
+  let devs = List.filter (fun dev -> String.find dev "-part" =
(-1)) devs in
+  let devs +    List.filter_map (
+      fun file ->
+        let dev = Unix_utils.Realpath.realpath (sprintf "%s/%s" path
file) in
+        (* Ignore non-/dev devices, and return without /dev/ prefix. *)
+        if String.is_prefix dev "/dev/" then
+          Some (String.sub dev 5 (String.length dev - 5))
+        else
+          None
+    ) devs in
   let devs = List.filter (
     fun dev ->
       String.is_prefix dev "sd" ||
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 5400adf64..fd87f6520 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -233,6 +233,9 @@ main (int argc, char *argv[])
   _umask (0);
 #endif
 
+  /* Initialize device name translations cache. */
+  device_name_translation_init ();
+
   /* Connect to virtio-serial channel. */
   if (!channel)
     channel = VIRTIO_SERIAL_CHANNEL;
@@ -322,6 +325,8 @@ main (int argc, char *argv[])
   /* Enter the main loop, reading and performing actions. */
   main_loop (sock);
 
+  device_name_translation_free ();
+
   exit (EXIT_SUCCESS);
 }
 
diff --git a/daemon/stubs-macros.h b/daemon/stubs-macros.h
index e8b6c500b..235e79472 100644
--- a/daemon/stubs-macros.h
+++ b/daemon/stubs-macros.h
@@ -30,11 +30,6 @@
  */
 #define RESOLVE_DEVICE(path,path_out,is_filein)                         \
   do {									\
-    if (!is_device_parameter ((path))) {                                \
-      if (is_filein) cancel_receive ();                                 \
-      reply_with_error ("%s: %s: expecting a device name", __func__,
(path)); \
-      return;							        \
-    }									\
     (path_out) = device_name_translation ((path));                      \
     if ((path_out) == NULL) {                                           \
       const int err = errno;                                            \
@@ -43,6 +38,11 @@
       reply_with_perror ("%s: %s", __func__, path);                  
\
       return;							        \
     }                                                                   \
+    if (!is_device_parameter ((path_out))) {                            \
+      if (is_filein) cancel_receive ();                                 \
+      reply_with_error ("%s: %s: expecting a device name", __func__,
(path)); \
+      return;							        \
+    }									\
   } while (0)
 
 /* All functions that take a mountable argument must call this macro.
diff --git a/lib/canonical-name.c b/lib/canonical-name.c
index 42a0fd2a6..efe45c5f1 100644
--- a/lib/canonical-name.c
+++ b/lib/canonical-name.c
@@ -37,6 +37,11 @@ guestfs_impl_canonical_device_name (guestfs_h *g, const char
*device)
       strchr (device+5, '/') == NULL && /* not an LV name */
       device[5] != 'm' && /* not /dev/md - RHBZ#1414682 */
       ((len = strcspn (device+5, "d")) > 0 && len <=
2)) {
+    /* NB!  These do not need to be translated by
+     * device_name_translation.  They will be translated if necessary
+     * when the caller uses them in APIs which go through to the
+     * daemon.
+     */
     ret = safe_asprintf (g, "/dev/sd%s", &device[5+len+1]);
   }
   else if (STRPREFIX (device, "/dev/mapper/") ||
-- 
2.25.0

This helps to debug problems with the new device name translation
code.  We can think about removing this later when the code is known
to work well.
---
 daemon/device-name-translation.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c
index 30173f5c2..b17f3c683 100644
--- a/daemon/device-name-translation.c
+++ b/daemon/device-name-translation.c
@@ -195,6 +195,10 @@ device_name_translation (const char *device)
       return NULL;
   }
 
+  /* If the device name is different, print the translation. */
+  if (STRNEQ (device, ret))
+    fprintf (stderr, "device name translated: %s -> %s\n", device,
ret);
+
   /* Now check the device is openable. */
   fd = open (ret, O_RDONLY|O_CLOEXEC);
   if (fd >= 0) {
@@ -281,5 +285,9 @@ reverse_device_name_translation (const char *device)
     }
   }
 
+  /* If the device name is different, print the translation. */
+  if (STRNEQ (device, ret))
+    fprintf (stderr, "reverse device name translated: %s -> %s\n",
device, ret);
+
   return ret;
 }
-- 
2.25.0

Appliance device names are not reliable since the kernel no longer
enumerates virtio-scsi devices serially.  Instead get the UUID of the
appliance and pass this as the parameter.

Note this requires supermin >= 5.1.18 (from around July 2017).
---
 docs/guestfs-building.pod |  2 +-
 lib/appliance-kcmdline.c  | 57 ++++++++++++++++++++++++++++++++++-----
 lib/guestfs-internal.h    |  2 +-
 lib/launch-direct.c       | 34 +----------------------
 lib/launch-libvirt.c      | 19 +++++++------
 m4/guestfs-appliance.m4   |  3 ++-
 6 files changed, 67 insertions(+), 50 deletions(-)

diff --git a/docs/guestfs-building.pod b/docs/guestfs-building.pod
index 005626515..8e3da1d32 100644
--- a/docs/guestfs-building.pod
+++ b/docs/guestfs-building.pod
@@ -83,7 +83,7 @@ I<Required>.
 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/appliance-kcmdline.c b/lib/appliance-kcmdline.c
index 8eb4999af..211cc4687 100644
--- a/lib/appliance-kcmdline.c
+++ b/lib/appliance-kcmdline.c
@@ -26,8 +26,10 @@
 #include <stdlib.h>
 #include <stdbool.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "c-ctype.h"
+#include "ignore-value.h"
 
 #include "guestfs.h"
 #include "guestfs-internal.h"
@@ -54,15 +56,53 @@
 #define EARLYPRINTK "earlyprintk=pl011,0x9000000"
 #endif
 
+COMPILE_REGEXP (re_uuid, "UUID=([-0-9a-f]+)", 0)
+
+static void
+read_uuid (guestfs_h *g, void *retv, const char *line, size_t len)
+{
+  char **ret = retv;
+
+  *ret = match1 (g, line, re_uuid);
+}
+
+/**
+ * Given a disk image containing an extX filesystem, return the UUID.
+ * The L<file(1)> command does the hard work.
+ */
+static char *
+get_root_uuid (guestfs_h *g, const char *appliance)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
+  char *ret = NULL;
+  int r;
+
+  guestfs_int_cmd_add_arg (cmd, "file");
+  guestfs_int_cmd_add_arg (cmd, "--");
+  guestfs_int_cmd_add_arg (cmd, appliance);
+  guestfs_int_cmd_set_stdout_callback (cmd, read_uuid, &ret, 0);
+  r = guestfs_int_cmd_run (cmd);
+  if (r == -1) {
+    if (ret) free (ret);
+    return NULL;
+  }
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    guestfs_int_external_command_failed (g, r, "file", NULL);
+    if (ret) free (ret);
+    return NULL;
+  }
+
+  return ret;
+}
+
 /**
  * Construct the Linux command line passed to the appliance.  This is
  * used by the C<direct> and C<libvirt> backends, and is simply
  * 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> parameter is the filename of the appliance
+ * (could be NULL) from which we obtain the root UUID.
  *
  * The C<flags> parameter can contain the following flags logically
  * or'd together (or 0):
@@ -80,7 +120,8 @@
  * 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,
 				    int flags)
 {
   CLEANUP_FREE_STRINGSBUF DECLARE_STRINGSBUF (argv);
@@ -164,8 +205,12 @@ 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) {
+    CLEANUP_FREE char *uuid = get_root_uuid (g, appliance);
+    if (!uuid)
+      return NULL;
+    guestfs_int_add_sprintf (g, &argv, "root=UUID=%s", uuid);
+  }
 
   /* SELinux - deprecated setting, never worked and should not be enabled. */
   if (g->selinux)
diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 8c6affa54..fe761a784 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -688,7 +688,7 @@ extern int guestfs_int_build_appliance (guestfs_h *g, char
**kernel, char **init
 const char *guestfs_int_get_cpu_model (int kvm);
 
 /* appliance-kcmdline.c */
-extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char
*appliance_dev, int flags);
+extern char *guestfs_int_appliance_command_line (guestfs_h *g, const char
*appliance, int flags);
 #define APPLIANCE_COMMAND_LINE_IS_TCG 1
 
 /* appliance-uefi.c */
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index 1718041a5..2de57ea32 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -61,8 +61,6 @@ struct backend_direct_data {
   char guestfsd_sock[UNIX_PATH_MAX]; /* Path to daemon socket. */
 };
 
-static char *make_appliance_dev (guestfs_h *g);
-
 static char *
 create_cow_overlay_direct (guestfs_h *g, void *datav, struct drive *drv)
 {
@@ -382,7 +380,6 @@ 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;
   uint32_t size;
   CLEANUP_FREE void *buf = NULL;
   struct hv_param *hp;
@@ -636,8 +633,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
       append_list ("scsi-hd");
       append_list ("drive=appliance");
     } end_list ();
-
-    appliance_dev = make_appliance_dev (g);
   }
 
   /* Create the virtio serial bus. */
@@ -697,7 +692,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   flags = 0;
   if (!has_kvm || force_tcg)
     flags |= APPLIANCE_COMMAND_LINE_IS_TCG;
-  append = guestfs_int_appliance_command_line (g, appliance_dev, flags);
+  append = guestfs_int_appliance_command_line (g, appliance, flags);
   arg ("-append", append);
 
   /* Note: custom command line parameters must come last so that
@@ -961,33 +956,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. */
-}
-
 static int
 shutdown_direct (guestfs_h *g, void *datav, int check_for_errors)
 {
diff --git a/lib/launch-libvirt.c b/lib/launch-libvirt.c
index 49786fdd9..cf83657a5 100644
--- a/lib/launch-libvirt.c
+++ b/lib/launch-libvirt.c
@@ -153,8 +153,9 @@ struct backend_libvirt_data {
  */
 struct libvirt_xml_params {
   struct backend_libvirt_data *data;
-  char *kernel;                 /* paths to kernel and initrd */
+  char *kernel;                 /* paths to kernel, initrd and appliance */
   char *initrd;
+  char *appliance;
   char *appliance_overlay;      /* path to qcow2 overlay backed by appliance */
   char appliance_dev[64];       /* appliance device name */
   size_t appliance_index;       /* index of appliance */
@@ -323,10 +324,10 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
     .data = data,
     .kernel = NULL,
     .initrd = NULL,
+    .appliance = NULL,
     .appliance_overlay = NULL,
   };
   CLEANUP_FREE xmlChar *xml = NULL;
-  CLEANUP_FREE char *appliance = NULL;
   struct sockaddr_un addr;
   struct drive *drv;
   size_t i;
@@ -486,18 +487,18 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   debug (g, "build appliance");
 
   if (guestfs_int_build_appliance (g, &params.kernel, &params.initrd,
-                                   &appliance) == -1)
+                                   &params.appliance) == -1)
     goto cleanup;
 
   guestfs_int_launch_send_progress (g, 3);
   TRACE0 (launch_build_libvirt_appliance_end);
 
   /* Note that appliance can be NULL if using the old-style appliance. */
-  if (appliance) {
+  if (params.appliance) {
 #ifndef APPLIANCE_FORMAT_AUTO
-    params.appliance_overlay = make_qcow2_overlay (g, appliance,
"raw");
+    params.appliance_overlay = make_qcow2_overlay (g, params.appliance,
"raw");
 #else
-    params.appliance_overlay = make_qcow2_overlay (g, appliance, NULL);
+    params.appliance_overlay = make_qcow2_overlay (g, params.appliance, NULL);
 #endif
     if (!params.appliance_overlay)
       goto cleanup;
@@ -714,7 +715,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
     goto cleanup;
   }
 
-  if (appliance)
+  if (params.appliance)
     guestfs_int_add_dummy_appliance_drive (g);
 
   TRACE0 (launch_libvirt_end);
@@ -726,6 +727,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
 
   free (params.kernel);
   free (params.initrd);
+  free (params.appliance);
   free (params.appliance_overlay);
 
   return 0;
@@ -751,6 +753,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
 
   free (params.kernel);
   free (params.initrd);
+  free (params.appliance);
   free (params.appliance_overlay);
 
   g->state = CONFIG;
@@ -1215,7 +1218,7 @@ 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,
flags);
 
   start_element ("os") {
     if (params->data->firmware)
diff --git a/m4/guestfs-appliance.m4 b/m4/guestfs-appliance.m4
index 827a83357..daed959af 100644
--- a/m4/guestfs-appliance.m4
+++ b/m4/guestfs-appliance.m4
@@ -34,7 +34,8 @@ 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.
+dnl We don't check it, but it must support the root=UUID=... notation.
 AC_PATH_PROG([SUPERMIN],[supermin],[no])
 
 dnl Pass supermin --packager-config option.
-- 
2.25.0

---
 daemon/device-name-translation.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/daemon/device-name-translation.c b/daemon/device-name-translation.c
index b17f3c683..39266fdd7 100644
--- a/daemon/device-name-translation.c
+++ b/daemon/device-name-translation.c
@@ -100,6 +100,12 @@ device_name_translation_init (void)
 
   /* This is the final cache size because we deleted entries above. */
   cache_size = j;
+
+  /* XXX */
+  for (i = 0; i < cache_size; ++i) {
+    fprintf (stderr, "device name translation cache: %zu -> %s\n",
+             i, cache[i]);
+  }
 }
 
 void
@@ -180,6 +186,8 @@ device_name_translation (const char *device)
     strcpy (dev, start);
     dev[len] = '\0';
 
+    fprintf (stderr, "DEVICE=%s START=%s LEN=%zu\n", device, start,
len);
+
     i = guestfs_int_drive_index (dev);
     if (i >= 0 && i < (ssize_t) cache_size) {
       /* Append the partition name if present. */
@@ -273,6 +281,8 @@ reverse_device_name_translation (const char *device)
         reply_with_perror ("asprintf");
         return NULL;
       }
+      fprintf (stderr, "REVERSE %s -> %s, index %zu, cache[%zu]=%s, len
%zu\n",
+               device, ret, i, i, cache[i], len);
       break;
     }
   }
-- 
2.25.0

After extensive testing of this patch on Fedora Rawhide, and RHEL 7
and 8, I pushed a slightly modified version (contains a fix for a
memory leak, and another which fixes the reverse name translation)
upstream.  The patch I pushed ran in a loop for 48+ hours with up to
255 disks without failing, and also passes the full test suite.

However there's no getting round it, it's still a risky patch.  So if
you see any regressions which might be related to disk naming /
numbering (if you are using Linux >= 5.6 or RHEL AV 8.2) then please
let us know.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top