drives: Centrally create overlays for read-only drives.

qemu has broken snapshot=on ... again.

Change the way that drives are created so that the backend no longer
has to use snapshot=on, <transient/> (which never worked), or UML's
corresponding COW-creation feature (also broken).

Instead of that, the src/drives.c code will create overlays when
required by calling into a new backend operation 'create_cow_overlay'.
This operation runs 'qemu-img create -b' or 'uml_mkcow' as determined
by the backend, and returns the name of the overlay.

The format of the overlay is still backend-specific because qemu needs
to use qcow2 and UML needs to use COW.

This patch also includes some factorization of the libvirt XML code.

This also drops the drv->priv (private per-drive data) field, since it
is no longer used by any backend.

This also moves the guestfs___drive_source_qemu_param utility
function, used & shared by the direct & libvirt backends only, into
src/launch-direct.c (from src/drives.c).

src/drives.c

@@ -37,8 +37,6 @@
 
 #include <pcre.h>
 
-#include <libxml/uri.h>
-
 #include "c-ctype.h"
 #include "ignore-value.h"
 
@@ -81,6 +79,38 @@ free_regexps (void)
   pcre_free (re_hostname_port);
 }
 
+static void free_drive_struct (struct drive *drv);
+static void free_drive_source (struct drive_source *src);
+
+/* For readonly drives, create an overlay to protect the original
+ * drive content.  Note we never need to clean up these overlays since
+ * they are created in the temporary directory and deleted when the
+ * handle is closed.
+ */
+static int
+create_overlay (guestfs_h *g, struct drive *drv)
+{
+  char *overlay;
+
+  assert (g->backend_ops != NULL);
+
+  if (g->backend_ops->create_cow_overlay == NULL) {
+    error (g, _("this backend does not support adding read-only drives"));
+    return -1;
+  }
+
+  debug (g, "creating COW overlay to protect original drive content");
+  overlay = g->backend_ops->create_cow_overlay (g, g->backend_data, drv);
+  if (overlay == NULL)
+    return -1;
+
+  if (drv->overlay)
+    free (drv->overlay);
+  drv->overlay = overlay;
+
+  return 0;
+}
+
 /* Create and free the 'drive' struct. */
 static struct drive *
 create_drive_file (guestfs_h *g, const char *path,
@@ -92,15 +122,20 @@ create_drive_file (guestfs_h *g, const char *path,
 
   drv->src.protocol = drive_protocol_file;
   drv->src.u.path = safe_strdup (g, path);
+  drv->src.format = format ? safe_strdup (g, format) : NULL;
 
   drv->readonly = readonly;
-  drv->format = format ? safe_strdup (g, format) : NULL;
   drv->iface = iface ? safe_strdup (g, iface) : NULL;
   drv->name = name ? safe_strdup (g, name) : NULL;
   drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
   drv->cachemode = cachemode ? safe_strdup (g, cachemode) : NULL;
 
-  drv->priv = drv->free_priv = NULL;
+  if (readonly) {
+    if (create_overlay (g, drv) == -1) {
+      free_drive_struct (drv);
+      return NULL;
+    }
+  }
 
   return drv;
 }
@@ -123,15 +158,20 @@ create_drive_non_file (guestfs_h *g,
   drv->src.u.exportname = safe_strdup (g, exportname);
   drv->src.username = username ? safe_strdup (g, username) : NULL;
   drv->src.secret = secret ? safe_strdup (g, secret) : NULL;
+  drv->src.format = format ? safe_strdup (g, format) : NULL;
 
   drv->readonly = readonly;
-  drv->format = format ? safe_strdup (g, format) : NULL;
   drv->iface = iface ? safe_strdup (g, iface) : NULL;
   drv->name = name ? safe_strdup (g, name) : NULL;
   drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
   drv->cachemode = cachemode ? safe_strdup (g, cachemode) : NULL;
 
-  drv->priv = drv->free_priv = NULL;
+  if (readonly) {
+    if (create_overlay (g, drv) == -1) {
+      free_drive_struct (drv);
+      return NULL;
+    }
+  }
 
   return drv;
 }
@@ -523,35 +563,49 @@ free_drive_servers (struct drive_server *servers, size_t nr_servers)
 static void
 free_drive_struct (struct drive *drv)
 {
-  guestfs___free_drive_source (&drv->src);
+  free_drive_source (&drv->src);
-  free (drv->format);
+  free (drv->overlay);
   free (drv->iface);
   free (drv->name);
   free (drv->disk_label);
   free (drv->cachemode);
 
-  if (drv->priv && drv->free_priv)
-    drv->free_priv (drv->priv);
-
   free (drv);
 }
 
+static const char *
+protocol_to_string (enum drive_protocol protocol)
+{
+  switch (protocol) {
+  case drive_protocol_file: return "file";
+  case drive_protocol_ftp: return "ftp";
+  case drive_protocol_ftps: return "ftps";
+  case drive_protocol_gluster: return "gluster";
+  case drive_protocol_http: return "http";
+  case drive_protocol_https: return "https";
+  case drive_protocol_iscsi: return "iscsi";
+  case drive_protocol_nbd: return "nbd";
+  case drive_protocol_rbd: return "rbd";
+  case drive_protocol_sheepdog: return "sheepdog";
+  case drive_protocol_ssh: return "ssh";
+  case drive_protocol_tftp: return "tftp";
+  }
+  abort ();
+}
+
 /* Convert a struct drive to a string for debugging.  The caller
  * must free this string.
  */
 static char *
 drive_to_string (guestfs_h *g, const struct drive *drv)
 {
-  CLEANUP_FREE char *p = NULL;
-
-  p = guestfs___drive_source_qemu_param (g, &drv->src);
-
   return safe_asprintf
-    (g, "%s%s%s%s%s%s%s%s%s%s%s%s",
+    (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s",
-     p,
+     drv->src.u.path,
      drv->readonly ? " readonly" : "",
-     drv->format ? " format=" : "",
+     drv->src.format ? " format=" : "",
-     drv->format ? : "",
+     drv->src.format ? : "",
+     protocol_to_string (drv->src.protocol),
      drv->iface ? " iface=" : "",
      drv->iface ? : "",
      drv->name ? " name=" : "",
@@ -1176,199 +1230,11 @@ guestfs__debug_drives (guestfs_h *g)
   return ret.argv;              /* caller frees */
 }
 
-/* The drive_source struct is also used in the backends, so we
+static void
- * also have these utility functions.
+free_drive_source (struct drive_source *src)
- */
-void
-guestfs___copy_drive_source (guestfs_h *g,
-                             const struct drive_source *src,
-                             struct drive_source *dest)
-{
-  size_t i;
-
-  dest->protocol = src->protocol;
-  dest->u.path = safe_strdup (g, src->u.path);
-  dest->nr_servers = src->nr_servers;
-  dest->servers = safe_calloc (g, src->nr_servers,
-                               sizeof (struct drive_server));
-  for (i = 0; i < src->nr_servers; ++i) {
-    dest->servers[i].transport = src->servers[i].transport;
-    if (src->servers[i].u.hostname)
-      dest->servers[i].u.hostname = safe_strdup (g, src->servers[i].u.hostname);
-    dest->servers[i].port = src->servers[i].port;
-  }
-}
-
-static char *
-make_uri (guestfs_h *g, const char *scheme, const char *user,
-          struct drive_server *server, const char *path)
-{
-  xmlURI uri = { .scheme = (char *) scheme,
-                 .path = (char *) path,
-                 .user = (char *) user };
-  CLEANUP_FREE char *query = NULL;
-
-  switch (server->transport) {
-  case drive_transport_none:
-  case drive_transport_tcp:
-    uri.server = server->u.hostname;
-    uri.port = server->port;
-    break;
-  case drive_transport_unix:
-    query = safe_asprintf (g, "socket=%s", server->u.socket);
-    uri.query_raw = query;
-    break;
-  }
-
-  return (char *) xmlSaveUri (&uri);
-}
-
-char *
-guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src)
-{
-  /* Note that the qemu parameter is the bit after "file=".  It is not
-   * escaped here, but would usually be escaped if passed to qemu as
-   * part of a full -drive parameter (but not for qemu-img).
-   */
-  switch (src->protocol) {
-  case drive_protocol_file:
-    /* We might need to rewrite the path if it contains a ':' character. */
-    if (src->u.path[0] == '/' || strchr (src->u.path, ':') == NULL)
-      return safe_strdup (g, src->u.path);
-    else
-      return safe_asprintf (g, "./%s", src->u.path);
-
-  case drive_protocol_ftp:
-    return make_uri (g, "ftp", src->username,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_ftps:
-    return make_uri (g, "ftps", src->username,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_gluster:
-    switch (src->servers[0].transport) {
-    case drive_transport_none:
-      return make_uri (g, "gluster", NULL, &src->servers[0], src->u.exportname);
-    case drive_transport_tcp:
-      return make_uri (g, "gluster+tcp",
-                       NULL, &src->servers[0], src->u.exportname);
-    case drive_transport_unix:
-      return make_uri (g, "gluster+unix", NULL, &src->servers[0], NULL);
-    }
-
-  case drive_protocol_http:
-    return make_uri (g, "http", src->username,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_https:
-    return make_uri (g, "https", src->username,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_iscsi:
-    return make_uri (g, "iscsi", NULL, &src->servers[0], src->u.exportname);
-
-  case drive_protocol_nbd: {
-    CLEANUP_FREE char *p = NULL;
-    char *ret;
-
-    switch (src->servers[0].transport) {
-    case drive_transport_none:
-    case drive_transport_tcp:
-      p = safe_asprintf (g, "nbd:%s:%d",
-                         src->servers[0].u.hostname, src->servers[0].port);
-      break;
-    case drive_transport_unix:
-      p = safe_asprintf (g, "nbd:unix:%s", src->servers[0].u.socket);
-      break;
-    }
-    assert (p);
-
-    if (STREQ (src->u.exportname, ""))
-      ret = safe_strdup (g, p);
-    else
-      /* Skip the mandatory leading '/' character. */
-      ret = safe_asprintf (g, "%s:exportname=%s", p, &src->u.exportname[1]);
-
-    return ret;
-  }
-
-  case drive_protocol_rbd: {
-    /* build the list of all the mon hosts */
-    CLEANUP_FREE char *mon_host = NULL, *username = NULL, *secret = NULL;
-    const char *auth;
-    size_t n = 0;
-    size_t i, j;
-
-    for (i = 0; i < src->nr_servers; i++) {
-      n += strlen (src->servers[i].u.hostname);
-      n += 8; /* for slashes, colons, & port numbers */
-    }
-    n++; /* for \0 */
-    mon_host = safe_malloc (g, n);
-    n = 0;
-    for (i = 0; i < src->nr_servers; i++) {
-      CLEANUP_FREE char *port = NULL;
-
-      for (j = 0; j < strlen (src->servers[i].u.hostname); j++)
-        mon_host[n++] = src->servers[i].u.hostname[j];
-      mon_host[n++] = '\\';
-      mon_host[n++] = ':';
-      port = safe_asprintf (g, "%d", src->servers[i].port);
-      for (j = 0; j < strlen (port); j++)
-        mon_host[n++] = port[j];
-
-      /* join each host with \; */
-      if (i != src->nr_servers - 1) {
-        mon_host[n++] = '\\';
-        mon_host[n++] = ';';
-      }
-    }
-    mon_host[n] = '\0';
-
-    if (src->username)
-        username = safe_asprintf (g, ":id=%s", src->username);
-    if (src->secret)
-        secret = safe_asprintf (g, ":key=%s", src->secret);
-    if (username || secret)
-        auth = ":auth_supported=cephx\\;none";
-    else
-        auth = ":auth_supported=none";
-
-    /* Skip the mandatory leading '/' character on exportname. */
-    return safe_asprintf (g, "rbd:%s:mon_host=%s%s%s%s",
-                          &src->u.exportname[1],
-                          mon_host,
-                          username ? username : "",
-                          auth,
-                          secret ? secret : "");
-  }
-
-  case drive_protocol_sheepdog:
-    /* Skip the mandatory leading '/' character on exportname. */
-    if (src->nr_servers == 0)
-      return safe_asprintf (g, "sheepdog:%s", &src->u.exportname[1]);
-    else                        /* XXX How to pass multiple hosts? */
-      return safe_asprintf (g, "sheepdog:%s:%d:%s",
-                            src->servers[0].u.hostname, src->servers[0].port,
-                            &src->u.exportname[1]);
-
-  case drive_protocol_ssh:
-    return make_uri (g, "ssh", src->username,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_tftp:
-    return make_uri (g, "tftp", src->username,
-                     &src->servers[0], src->u.exportname);
-  }
-
-  abort ();
-}
-
-void
-guestfs___free_drive_source (struct drive_source *src)
 {
   if (src) {
+    free (src->format);
     free (src->u.path);
     free (src->username);
     free (src->secret);

src/guestfs-internal.h

@@ -209,6 +209,9 @@ struct drive_server {
 struct drive_source {
   enum drive_protocol protocol;
 
+  /* Format (eg. raw, qcow2).  NULL = autodetect. */
+  char *format;
+
   /* This field is always non-NULL.  It may be an empty string. */
   union {
     char *path;                 /* path to file (file) */
@@ -228,19 +231,27 @@ struct drive_source {
   char *secret;
 };
 
+/* There is one 'struct drive' per drive, including hot-plugged drives. */
 struct drive {
+  /* Original source of the drive, eg. file:..., http:... */
   struct drive_source src;
 
+  /* If the drive is readonly, then an overlay [a local file] is
+   * created before launch to protect the original drive content, and
+   * the filename is stored here.  Backends should open this file if
+   * it is non-NULL, else consult the original source above.
+   *
+   * Note that the overlay is in a backend-specific format, probably
+   * different from the source format.  eg. qcow2, UML COW.
+   */
+  char *overlay;
+
+  /* Various per-drive flags. */
   bool readonly;
-  char *format;
   char *iface;
   char *name;
   char *disk_label;
   char *cachemode;
-
-  /* Data used by the backend. */
-  void *priv;
-  void (*free_priv) (void *);
 };
 
 /* Extra hv parameters (from guestfs_config). */
@@ -262,6 +273,12 @@ struct backend_ops {
    */
   size_t data_size;
 
+  /* Create a COW overlay on top of a drive.  This must be a local
+   * file, created in the temporary directory.  This is called when
+   * the drive is added to the handle.
+   */
+  char *(*create_cow_overlay) (guestfs_h *g, void *data, struct drive *drv);
+
   /* Launch and shut down. */
   int (*launch) (guestfs_h *g, void *data, const char *arg);
   int (*shutdown) (guestfs_h *g, void *data, int check_for_errors);
@@ -695,9 +712,6 @@ extern size_t guestfs___checkpoint_drives (guestfs_h *g);
 extern void guestfs___rollback_drives (guestfs_h *g, size_t);
 extern void guestfs___add_dummy_appliance_drive (guestfs_h *g);
 extern void guestfs___free_drives (guestfs_h *g);
-extern void guestfs___copy_drive_source (guestfs_h *g, const struct drive_source *src, struct drive_source *dest);
-extern char *guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src);
-extern void guestfs___free_drive_source (struct drive_source *src);
 
 /* appliance.c */
 extern int guestfs___build_appliance (guestfs_h *g, char **kernel, char **dtb, char **initrd, char **appliance);
@@ -813,4 +827,7 @@ extern void guestfs___cmd_close (struct command *);
 #endif
 extern void guestfs___cleanup_cmd_close (struct command **);
 
+/* launch-direct.c */
+extern char *guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src);
+
 #endif /* GUESTFS_INTERNAL_H_ */

src/launch-direct.c

@@ -33,9 +33,12 @@
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <grp.h>
+#include <assert.h>
 
 #include <pcre.h>
 
+#include <libxml/uri.h>
+
 #include "cloexec.h"
 #include "ignore-value.h"
 
@@ -101,6 +104,53 @@ static int qemu_supports_device (guestfs_h *g, struct backend_direct_data *, con
 static int qemu_supports_virtio_scsi (guestfs_h *g, struct backend_direct_data *);
 static char *qemu_escape_param (guestfs_h *g, const char *param);
 
+static char *
+create_cow_overlay_direct (guestfs_h *g, void *datav, struct drive *drv)
+{
+  char *overlay = NULL;
+  CLEANUP_FREE char *backing_drive = NULL;
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g);
+  int r;
+
+  backing_drive = guestfs___drive_source_qemu_param (g, &drv->src);
+  if (!backing_drive)
+    goto error;
+
+  if (guestfs___lazy_make_tmpdir (g) == -1)
+    goto error;
+
+  overlay = safe_asprintf (g, "%s/overlay%d", g->tmpdir, ++g->unique);
+
+  guestfs___cmd_add_arg (cmd, "qemu-img");
+  guestfs___cmd_add_arg (cmd, "create");
+  guestfs___cmd_add_arg (cmd, "-f");
+  guestfs___cmd_add_arg (cmd, "qcow2");
+  guestfs___cmd_add_arg (cmd, "-b");
+  guestfs___cmd_add_arg (cmd, backing_drive);
+  if (drv->src.format) {
+    guestfs___cmd_add_arg (cmd, "-o");
+    guestfs___cmd_add_arg_format (cmd, "backing_fmt=%s", drv->src.format);
+  }
+  guestfs___cmd_add_arg (cmd, overlay);
+  r = guestfs___cmd_run (cmd);
+  if (r == -1)
+    goto error;
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    guestfs___external_command_failed (g, r, "qemu-img create", backing_drive);
+    goto error;
+  }
+
+  /* Caller sets g->overlay in the handle to this, and then manages
+   * the memory.
+   */
+  return overlay;
+
+ error:
+  free (overlay);
+
+  return NULL;
+}
+
 #ifdef QEMU_OPTIONS
 /* Like 'add_cmdline' but allowing a shell-quoted string of zero or
  * more options.  XXX The unquoting is not very clever.
@@ -470,23 +520,35 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   ITER_DRIVES (g, i, drv) {
     CLEANUP_FREE char *file = NULL, *escaped_file = NULL, *param = NULL;
 
-    /* Make the file= parameter. */
+    if (!drv->overlay) {
-    file = guestfs___drive_source_qemu_param (g, &drv->src);
+      /* Make the file= parameter. */
-    escaped_file = qemu_escape_param (g, file);
+      file = guestfs___drive_source_qemu_param (g, &drv->src);
+      escaped_file = qemu_escape_param (g, file);
 
-    /* Make the first part of the -drive parameter, everything up to
+      /* Make the first part of the -drive parameter, everything up to
-     * the if=... at the end.
+       * the if=... at the end.
-     */
+       */
-    param = safe_asprintf
+      param = safe_asprintf
-      (g, "file=%s%s,cache=%s%s%s%s%s,id=hd%zu",
+        (g, "file=%s%s,cache=%s%s%s%s%s,id=hd%zu",
-       escaped_file,
+         escaped_file,
-       drv->readonly ? ",snapshot=on" : "",
+         drv->readonly ? ",snapshot=on" : "",
-       drv->cachemode ? drv->cachemode : "writeback",
+         drv->cachemode ? drv->cachemode : "writeback",
-       drv->format ? ",format=" : "",
+         drv->src.format ? ",format=" : "",
-       drv->format ? drv->format : "",
+         drv->src.format ? drv->src.format : "",
-       drv->disk_label ? ",serial=" : "",
+         drv->disk_label ? ",serial=" : "",
-       drv->disk_label ? drv->disk_label : "",
+         drv->disk_label ? drv->disk_label : "",
-       i);
+         i);
+    }
+    else {
+      /* Writable qcow2 overlay on top of read-only drive. */
+      escaped_file = qemu_escape_param (g, drv->overlay);
+      param = safe_asprintf
+        (g, "file=%s,cache=unsafe,format=qcow2%s%s,id=hd%zu",
+         escaped_file,
+         drv->disk_label ? ",serial=" : "",
+         drv->disk_label ? drv->disk_label : "",
+         i);
+    }
 
     /* If there's an explicit 'iface', use it.  Otherwise default to
      * virtio-scsi if available.  Otherwise default to virtio-blk.
@@ -1130,6 +1192,186 @@ qemu_escape_param (guestfs_h *g, const char *param)
   return ret;
 }
 
+static char *
+make_uri (guestfs_h *g, const char *scheme, const char *user,
+          struct drive_server *server, const char *path)
+{
+  xmlURI uri = { .scheme = (char *) scheme,
+                 .path = (char *) path,
+                 .user = (char *) user };
+  CLEANUP_FREE char *query = NULL;
+
+  switch (server->transport) {
+  case drive_transport_none:
+  case drive_transport_tcp:
+    uri.server = server->u.hostname;
+    uri.port = server->port;
+    break;
+  case drive_transport_unix:
+    query = safe_asprintf (g, "socket=%s", server->u.socket);
+    uri.query_raw = query;
+    break;
+  }
+
+  return (char *) xmlSaveUri (&uri);
+}
+
+/* Useful function to format a drive + protocol for qemu.  Also shared
+ * with launch-libvirt.c.
+ *
+ * Note that the qemu parameter is the bit after "file=".  It is not
+ * escaped here, but would usually be escaped if passed to qemu as
+ * part of a full -drive parameter (but not for qemu-img).
+ */
+char *
+guestfs___drive_source_qemu_param (guestfs_h *g, const struct drive_source *src)
+{
+  char *path;
+
+  switch (src->protocol) {
+  case drive_protocol_file:
+    /* We have to convert the path to an absolute path, since
+     * otherwise qemu will look for the backing file relative to the
+     * overlay (which is located in g->tmpdir).
+     *
+     * As a side-effect this deals with paths that contain ':' since
+     * qemu will not process the ':' if the path begins with '/'.
+     */
+    path = realpath (src->u.path, NULL);
+    if (path == NULL) {
+      perrorf (g, _("realpath: could not convert '%s' to absolute path"),
+               src->u.path);
+      return NULL;
+    }
+    return path;
+
+  case drive_protocol_ftp:
+    return make_uri (g, "ftp", src->username,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_ftps:
+    return make_uri (g, "ftps", src->username,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_gluster:
+    switch (src->servers[0].transport) {
+    case drive_transport_none:
+      return make_uri (g, "gluster", NULL, &src->servers[0], src->u.exportname);
+    case drive_transport_tcp:
+      return make_uri (g, "gluster+tcp",
+                       NULL, &src->servers[0], src->u.exportname);
+    case drive_transport_unix:
+      return make_uri (g, "gluster+unix", NULL, &src->servers[0], NULL);
+    }
+
+  case drive_protocol_http:
+    return make_uri (g, "http", src->username,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_https:
+    return make_uri (g, "https", src->username,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_iscsi:
+    return make_uri (g, "iscsi", NULL, &src->servers[0], src->u.exportname);
+
+  case drive_protocol_nbd: {
+    CLEANUP_FREE char *p = NULL;
+    char *ret;
+
+    switch (src->servers[0].transport) {
+    case drive_transport_none:
+    case drive_transport_tcp:
+      p = safe_asprintf (g, "nbd:%s:%d",
+                         src->servers[0].u.hostname, src->servers[0].port);
+      break;
+    case drive_transport_unix:
+      p = safe_asprintf (g, "nbd:unix:%s", src->servers[0].u.socket);
+      break;
+    }
+    assert (p);
+
+    if (STREQ (src->u.exportname, ""))
+      ret = safe_strdup (g, p);
+    else
+      /* Skip the mandatory leading '/' character. */
+      ret = safe_asprintf (g, "%s:exportname=%s", p, &src->u.exportname[1]);
+
+    return ret;
+  }
+
+  case drive_protocol_rbd: {
+    /* build the list of all the mon hosts */
+    CLEANUP_FREE char *mon_host = NULL, *username = NULL, *secret = NULL;
+    const char *auth;
+    size_t n = 0;
+    size_t i, j;
+
+    for (i = 0; i < src->nr_servers; i++) {
+      n += strlen (src->servers[i].u.hostname);
+      n += 8; /* for slashes, colons, & port numbers */
+    }
+    n++; /* for \0 */
+    mon_host = safe_malloc (g, n);
+    n = 0;
+    for (i = 0; i < src->nr_servers; i++) {
+      CLEANUP_FREE char *port = NULL;
+
+      for (j = 0; j < strlen (src->servers[i].u.hostname); j++)
+        mon_host[n++] = src->servers[i].u.hostname[j];
+      mon_host[n++] = '\\';
+      mon_host[n++] = ':';
+      port = safe_asprintf (g, "%d", src->servers[i].port);
+      for (j = 0; j < strlen (port); j++)
+        mon_host[n++] = port[j];
+
+      /* join each host with \; */
+      if (i != src->nr_servers - 1) {
+        mon_host[n++] = '\\';
+        mon_host[n++] = ';';
+      }
+    }
+    mon_host[n] = '\0';
+
+    if (src->username)
+        username = safe_asprintf (g, ":id=%s", src->username);
+    if (src->secret)
+        secret = safe_asprintf (g, ":key=%s", src->secret);
+    if (username || secret)
+        auth = ":auth_supported=cephx\\;none";
+    else
+        auth = ":auth_supported=none";
+
+    /* Skip the mandatory leading '/' character on exportname. */
+    return safe_asprintf (g, "rbd:%s:mon_host=%s%s%s%s",
+                          &src->u.exportname[1],
+                          mon_host,
+                          username ? username : "",
+                          auth,
+                          secret ? secret : "");
+  }
+
+  case drive_protocol_sheepdog:
+    /* Skip the mandatory leading '/' character on exportname. */
+    if (src->nr_servers == 0)
+      return safe_asprintf (g, "sheepdog:%s", &src->u.exportname[1]);
+    else                        /* XXX How to pass multiple hosts? */
+      return safe_asprintf (g, "sheepdog:%s:%d:%s",
+                            src->servers[0].u.hostname, src->servers[0].port,
+                            &src->u.exportname[1]);
+
+  case drive_protocol_ssh:
+    return make_uri (g, "ssh", src->username,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_tftp:
+    return make_uri (g, "tftp", src->username,
+                     &src->servers[0], src->u.exportname);
+  }
+
+  abort ();
+}
+
 static int
 shutdown_direct (guestfs_h *g, void *datav, int check_for_errors)
 {
@@ -1196,6 +1438,7 @@ max_disks_direct (guestfs_h *g, void *datav)
 
 static struct backend_ops backend_direct_ops = {
   .data_size = sizeof (struct backend_direct_data),
+  .create_cow_overlay = create_cow_overlay_direct,
   .launch = launch_direct,
   .shutdown = shutdown_direct,
   .get_pid = get_pid_direct,

src/launch-libvirt.c

@@ -108,18 +108,6 @@ struct backend_libvirt_data {
   char name[DOMAIN_NAME_LEN];   /* random name */
 };
 
-/* Pointed to by 'struct drive *' -> priv field. */
-struct drive_libvirt {
-  /* The drive that we actually add.  If using an overlay, then this
-   * might be different from drive->src.  Call it 'real_src' so we
-   * don't confuse accesses to this with accesses to 'drive->src'.
-   */
-  struct drive_source real_src;
-
-  /* The format of the drive we add. */
-  char *format;
-};
-
 /* Parameters passed to construct_libvirt_xml and subfunctions.  We
  * keep them all in a structure for convenience!
  */
@@ -146,9 +134,6 @@ static void libvirt_error (guestfs_h *g, const char *fs, ...) __attribute__((for
 static int is_custom_hv (guestfs_h *g);
 static int is_blk (const char *path);
 static void ignore_errors (void *ignore, virErrorPtr ignore2);
-static char *make_qcow2_overlay (guestfs_h *g, const char *backing_device, const char *format, const char *selinux_imagelabel);
-static int make_drive_priv (guestfs_h *g, struct drive *drv, const char *selinux_imagelabel);
-static void drive_free_priv (void *);
 static void set_socket_create_context (guestfs_h *g);
 static void clear_socket_create_context (guestfs_h *g);
 
@@ -156,6 +141,80 @@ static void clear_socket_create_context (guestfs_h *g);
 static void selinux_warning (guestfs_h *g, const char *func, const char *selinux_op, const char *data);
 #endif
 
+static char *
+make_qcow2_overlay (guestfs_h *g, const char *backing_drive,
+                    const char *format)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g);
+  char *overlay = NULL;
+  int r;
+
+  if (guestfs___lazy_make_tmpdir (g) == -1)
+    return NULL;
+
+  overlay = safe_asprintf (g, "%s/overlay%d", g->tmpdir, ++g->unique);
+
+  guestfs___cmd_add_arg (cmd, "qemu-img");
+  guestfs___cmd_add_arg (cmd, "create");
+  guestfs___cmd_add_arg (cmd, "-f");
+  guestfs___cmd_add_arg (cmd, "qcow2");
+  guestfs___cmd_add_arg (cmd, "-b");
+  guestfs___cmd_add_arg (cmd, backing_drive);
+  if (format) {
+    guestfs___cmd_add_arg (cmd, "-o");
+    guestfs___cmd_add_arg_format (cmd, "backing_fmt=%s", format);
+  }
+  guestfs___cmd_add_arg (cmd, overlay);
+  r = guestfs___cmd_run (cmd);
+  if (r == -1) {
+    free (overlay);
+    return NULL;
+  }
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    guestfs___external_command_failed (g, r, "qemu-img create", backing_drive);
+    free (overlay);
+    return NULL;
+  }
+
+  return overlay;
+}
+
+static char *
+create_cow_overlay_libvirt (guestfs_h *g, void *datav, struct drive *drv)
+{
+  struct backend_libvirt_data *data = datav;
+  CLEANUP_FREE char *backing_drive = NULL;
+  char *overlay = NULL;
+
+  backing_drive = guestfs___drive_source_qemu_param (g, &drv->src);
+  if (!backing_drive)
+    goto error;
+
+  overlay = make_qcow2_overlay (g, backing_drive, drv->src.format);
+  if (!overlay)
+    goto error;
+
+#if HAVE_LIBSELINUX
+  if (data->selinux_imagelabel) {
+    debug (g, "setting SELinux label on %s to %s",
+           overlay, data->selinux_imagelabel);
+    if (setfilecon (overlay,
+                    (security_context_t) data->selinux_imagelabel) == -1)
+      selinux_warning (g, __func__, "setfilecon", overlay);
+  }
+#endif
+
+  /* Caller sets g->overlay in the handle to this, and then manages
+   * the memory.
+   */
+  return overlay;
+
+ error:
+  free (overlay);
+
+  return NULL;
+}
+
 static int
 launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri)
 {
@@ -178,8 +237,6 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri)
   int r;
   uint32_t size;
   CLEANUP_FREE void *buf = NULL;
-  struct drive *drv;
-  size_t i;
 
   params.current_proc_is_root = geteuid () == 0;
 
@@ -274,19 +331,11 @@ launch_libvirt (guestfs_h *g, void *datav, const char *libvirt_uri)
 
   /* Note that appliance can be NULL if using the old-style appliance. */
   if (appliance) {
-    params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw", NULL);
+    params.appliance_overlay = make_qcow2_overlay (g, appliance, "raw");
     if (!params.appliance_overlay)
       goto cleanup;
   }
 
-  /* Set up the drv->priv part of the struct.  A side-effect of this
-   * may be that we create qcow2 overlays for drives.
-   */
-  ITER_DRIVES (g, i, drv) {
-    if (make_drive_priv (g, drv, data->selinux_imagelabel) == -1)
-      goto cleanup;
-  }
-
   TRACE0 (launch_build_libvirt_qcow2_overlay_end);
 
   /* Using virtio-serial, we need to create a local Unix domain socket
@@ -746,6 +795,9 @@ static int construct_libvirt_xml_lifecycle (guestfs_h *g, const struct libvirt_x
 static int construct_libvirt_xml_devices (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_qemu_cmdline (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_disk (guestfs_h *g, const struct backend_libvirt_data *data, xmlTextWriterPtr xo, struct drive *drv, size_t drv_index);
+static int construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr xo, size_t drv_index);
+static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, xmlTextWriterPtr xo, const char *format, const char *cachemode);
+static int construct_libvirt_xml_disk_address (guestfs_h *g, xmlTextWriterPtr xo, size_t drv_index);
 static int construct_libvirt_xml_disk_source_hosts (guestfs_h *g, xmlTextWriterPtr xo, const struct drive_source *src);
 static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const struct backend_libvirt_data *data, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_appliance (guestfs_h *g, const struct libvirt_xml_params *params, xmlTextWriterPtr xo);
@@ -756,7 +808,8 @@ static int construct_libvirt_xml_appliance (guestfs_h *g, const struct libvirt_x
  */
 #define XMLERROR_RET(code,e,ret) do {                                   \
     if ((e) == (code)) {                                                \
-      perrorf (g, _("error constructing libvirt XML at \"%s\""),        \
+      perrorf (g, _("%s:%d: error constructing libvirt XML at \"%s\""), \
+               __FILE__, __LINE__,                                      \
                #e);                                                     \
       return (ret);                                                     \
     }                                                                   \
@@ -1111,12 +1164,10 @@ construct_libvirt_xml_disk (guestfs_h *g,
                             xmlTextWriterPtr xo,
                             struct drive *drv, size_t drv_index)
 {
-  char drive_name[64] = "sd";
   const char *protocol_str;
-  char scsi_target[64];
+  CLEANUP_FREE char *path = NULL;
-  struct drive_libvirt *drv_priv = (struct drive_libvirt *) drv->priv;
-  CLEANUP_FREE char *format = NULL;
   int is_host_device;
+  CLEANUP_FREE char *format = NULL;
 
   /* XXX We probably could support this if we thought about it some more. */
   if (drv->iface) {
@@ -1124,116 +1175,207 @@ construct_libvirt_xml_disk (guestfs_h *g,
     return -1;
   }
 
-  guestfs___drive_name (drv_index, &drive_name[2]);
-  snprintf (scsi_target, sizeof scsi_target, "%zu", drv_index);
-
   XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "disk"));
   XMLERROR (-1,
             xmlTextWriterWriteAttribute (xo, BAD_CAST "device",
                                          BAD_CAST "disk"));
 
-  switch (drv_priv->real_src.protocol) {
+  if (drv->overlay) {
-  case drive_protocol_file:
+    /* Overlay to protect read-only backing disk.  The format of the
-    /* Change the libvirt XML according to whether the host path is
+     * overlay is always qcow2.
-     * a device or a file.  For devices, use:
-     *   <disk type=block device=disk>
-     *     <source dev=[path]>
-     * For files, use:
-     *   <disk type=file device=disk>
-     *     <source file=[path]>
      */
-    is_host_device = is_blk (drv_priv->real_src.u.path);
-
-    if (!is_host_device) {
-      XMLERROR (-1,
-                xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-                                             BAD_CAST "file"));
-
-      XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "source"));
-      XMLERROR (-1,
-                xmlTextWriterWriteAttribute (xo, BAD_CAST "file",
-                                             BAD_CAST drv_priv->real_src.u.path));
-      if (construct_libvirt_xml_disk_source_seclabel (g, data, xo) == -1)
-        return -1;
-      XMLERROR (-1, xmlTextWriterEndElement (xo));
-    }
-    else {
-      XMLERROR (-1,
-                xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-                                             BAD_CAST "block"));
-
-      XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "source"));
-      XMLERROR (-1,
-                xmlTextWriterWriteAttribute (xo, BAD_CAST "dev",
-                                             BAD_CAST drv_priv->real_src.u.path));
-      if (construct_libvirt_xml_disk_source_seclabel (g, data, xo) == -1)
-        return -1;
-      XMLERROR (-1, xmlTextWriterEndElement (xo));
-    }
-    break;
-
-    /* For network protocols:
-     *   <disk type=network device=disk>
-     *     <source protocol=[protocol] [name=exportname]>
-     * and then zero or more of:
-     *       <host name='example.com' port='10809'/>
-     * or:
-     *       <host transport='unix' socket='/path/to/socket'/>
-     */
-  case drive_protocol_gluster:
-    protocol_str = "gluster"; goto network_protocols;
-  case drive_protocol_iscsi:
-    protocol_str = "iscsi"; goto network_protocols;
-  case drive_protocol_nbd:
-    protocol_str = "nbd"; goto network_protocols;
-  case drive_protocol_rbd:
-    protocol_str = "rbd"; goto network_protocols;
-  case drive_protocol_sheepdog:
-    protocol_str = "sheepdog"; goto network_protocols;
-  case drive_protocol_ssh:
-    protocol_str = "ssh";
-    /*FALLTHROUGH*/
-  network_protocols:
     XMLERROR (-1,
               xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-                                           BAD_CAST "network"));
+                                           BAD_CAST "file"));
+
     XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "source"));
     XMLERROR (-1,
-              xmlTextWriterWriteAttribute (xo, BAD_CAST "protocol",
+              xmlTextWriterWriteAttribute (xo, BAD_CAST "file",
-                                           BAD_CAST protocol_str));
+                                           BAD_CAST drv->overlay));
-    if (STRNEQ (drv_priv->real_src.u.exportname, ""))
-      XMLERROR (-1,
-                xmlTextWriterWriteAttribute (xo, BAD_CAST "name",
-                                             BAD_CAST drv_priv->real_src.u.exportname));
-    if (construct_libvirt_xml_disk_source_hosts (g, xo,
-                                                 &drv_priv->real_src) == -1)
-      return -1;
     if (construct_libvirt_xml_disk_source_seclabel (g, data, xo) == -1)
       return -1;
     XMLERROR (-1, xmlTextWriterEndElement (xo));
-    if (drv_priv->real_src.username != NULL) {
-      XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "auth"));
-      XMLERROR (-1,
-                xmlTextWriterWriteAttribute (xo, BAD_CAST "username",
-                                             BAD_CAST drv_priv->real_src.username));
-      /* TODO: write the drive secret, after first storing it separately
-       * in libvirt
-       */
-      XMLERROR (-1, xmlTextWriterEndElement (xo));
-    }
-    break;
 
-    /* libvirt doesn't support the qemu curl driver yet.  Give a
+    if (construct_libvirt_xml_disk_target (g, xo, drv_index) == -1)
-     * reasonable error message instead of trying and failing.
+      return -1;
-     */
+
-  case drive_protocol_ftp:
+    if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2", "unsafe") == -1)
-  case drive_protocol_ftps:
+      return -1;
-  case drive_protocol_http:
-  case drive_protocol_https:
-  case drive_protocol_tftp:
-    error (g, _("libvirt does not support the qemu curl driver protocols (ftp, http, etc.); try setting LIBGUESTFS_BACKEND=direct"));
-    return -1;
   }
+  else {
+    /* Not an overlay, a writable disk. */
+
+    switch (drv->src.protocol) {
+    case drive_protocol_file:
+      /* Change the libvirt XML according to whether the host path is
+       * a device or a file.  For devices, use:
+       *   <disk type=block device=disk>
+       *     <source dev=[path]>
+       * For files, use:
+       *   <disk type=file device=disk>
+       *     <source file=[path]>
+       */
+      is_host_device = is_blk (drv->src.u.path);
+
+      if (!is_host_device) {
+        path = realpath (drv->src.u.path, NULL);
+        if (path == NULL) {
+          perrorf (g, _("realpath: could not convert '%s' to absolute path"),
+                   drv->src.u.path);
+          return -1;
+        }
+
+        XMLERROR (-1,
+                  xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
+                                               BAD_CAST "file"));
+
+        XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "source"));
+        XMLERROR (-1,
+                  xmlTextWriterWriteAttribute (xo, BAD_CAST "file",
+                                               BAD_CAST path));
+        if (construct_libvirt_xml_disk_source_seclabel (g, data, xo) == -1)
+          return -1;
+        XMLERROR (-1, xmlTextWriterEndElement (xo));
+      }
+      else {
+        XMLERROR (-1,
+                  xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
+                                               BAD_CAST "block"));
+
+        XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "source"));
+        XMLERROR (-1,
+                  xmlTextWriterWriteAttribute (xo, BAD_CAST "dev",
+                                               BAD_CAST drv->src.u.path));
+        if (construct_libvirt_xml_disk_source_seclabel (g, data, xo) == -1)
+          return -1;
+        XMLERROR (-1, xmlTextWriterEndElement (xo));
+      }
+      break;
+
+      /* For network protocols:
+       *   <disk type=network device=disk>
+       *     <source protocol=[protocol] [name=exportname]>
+       * and then zero or more of:
+       *       <host name='example.com' port='10809'/>
+       * or:
+       *       <host transport='unix' socket='/path/to/socket'/>
+       */
+    case drive_protocol_gluster:
+      protocol_str = "gluster"; goto network_protocols;
+    case drive_protocol_iscsi:
+      protocol_str = "iscsi"; goto network_protocols;
+    case drive_protocol_nbd:
+      protocol_str = "nbd"; goto network_protocols;
+    case drive_protocol_rbd:
+      protocol_str = "rbd"; goto network_protocols;
+    case drive_protocol_sheepdog:
+      protocol_str = "sheepdog"; goto network_protocols;
+    case drive_protocol_ssh:
+      protocol_str = "ssh";
+      /*FALLTHROUGH*/
+    network_protocols:
+      XMLERROR (-1,
+                xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
+                                             BAD_CAST "network"));
+      XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "source"));
+      XMLERROR (-1,
+                xmlTextWriterWriteAttribute (xo, BAD_CAST "protocol",
+                                             BAD_CAST protocol_str));
+      if (STRNEQ (drv->src.u.exportname, ""))
+        XMLERROR (-1,
+                  xmlTextWriterWriteAttribute (xo, BAD_CAST "name",
+                                               BAD_CAST drv->src.u.exportname));
+      if (construct_libvirt_xml_disk_source_hosts (g, xo,
+                                                   &drv->src) == -1)
+        return -1;
+      if (construct_libvirt_xml_disk_source_seclabel (g, data, xo) == -1)
+        return -1;
+      XMLERROR (-1, xmlTextWriterEndElement (xo));
+      if (drv->src.username != NULL) {
+        XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "auth"));
+        XMLERROR (-1,
+                  xmlTextWriterWriteAttribute (xo, BAD_CAST "username",
+                                               BAD_CAST drv->src.username));
+        /* TODO: write the drive secret, after first storing it separately
+         * in libvirt
+         */
+        XMLERROR (-1, xmlTextWriterEndElement (xo));
+      }
+      break;
+
+      /* libvirt doesn't support the qemu curl driver yet.  Give a
+       * reasonable error message instead of trying and failing.
+       */
+    case drive_protocol_ftp:
+    case drive_protocol_ftps:
+    case drive_protocol_http:
+    case drive_protocol_https:
+    case drive_protocol_tftp:
+      error (g, _("libvirt does not support the qemu curl driver protocols (ftp, http, etc.); try setting LIBGUESTFS_BACKEND=direct"));
+      return -1;
+    }
+
+    if (construct_libvirt_xml_disk_target (g, xo, drv_index) == -1)
+      return -1;
+
+    if (drv->src.format)
+      format = safe_strdup (g, drv->src.format);
+    else if (drv->src.protocol == drive_protocol_file) {
+      /* libvirt has disabled the feature of detecting the disk format,
+       * unless the administrator sets allow_disk_format_probing=1 in
+       * qemu.conf.  There is no way to detect if this option is set, so we
+       * have to do format detection here using qemu-img and pass that to
+       * libvirt.
+       *
+       * This is still a security issue, so in most cases it is recommended
+       * the users pass the format to libguestfs which will faithfully pass
+       * that to libvirt and this function won't be used.
+       */
+      format = guestfs_disk_format (g, drv->src.u.path);
+      if (!format)
+        return -1;
+
+      if (STREQ (format, "unknown")) {
+        error (g, _("could not auto-detect the format.\n"
+                    "If the format is known, pass the format to libguestfs, eg. using the\n"
+                    "'--format' option, or via the optional 'format' argument to 'add-drive'."));
+        return -1;
+      }
+    }
+    else {
+      error (g, _("could not auto-detect the format when using a non-file protocol.\n"
+                  "If the format is known, pass the format to libguestfs, eg. using the\n"
+                  "'--format' option, or via the optional 'format' argument to 'add-drive'."));
+      return -1;
+    }
+
+    if (construct_libvirt_xml_disk_driver_qemu (g, xo, format,
+                                                drv->cachemode ? : "writeback")
+        == -1)
+      return -1;
+  }
+
+  if (drv->disk_label) {
+    XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "serial"));
+    XMLERROR (-1, xmlTextWriterWriteString (xo, BAD_CAST drv->disk_label));
+    XMLERROR (-1, xmlTextWriterEndElement (xo));
+  }
+
+  if (construct_libvirt_xml_disk_address (g, xo, drv_index) == -1)
+    return -1;
+
+  XMLERROR (-1, xmlTextWriterEndElement (xo)); /* </disk> */
+
+  return 0;
+}
+
+static int
+construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr xo,
+                                   size_t drv_index)
+{
+  char drive_name[64] = "sd";
+
+  guestfs___drive_name (drv_index, &drive_name[2]);
 
   XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "target"));
   XMLERROR (-1,
@@ -1244,61 +1386,36 @@ construct_libvirt_xml_disk (guestfs_h *g,
                                          BAD_CAST "scsi"));
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
+  return 0;
+}
+
+static int
+construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, xmlTextWriterPtr xo,
+                                        const char *format,
+                                        const char *cachemode)
+{
   XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "driver"));
   XMLERROR (-1,
             xmlTextWriterWriteAttribute (xo, BAD_CAST "name",
                                          BAD_CAST "qemu"));
-  if (drv_priv->format) {
+  XMLERROR (-1,
-    XMLERROR (-1,
+            xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-              xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
+                                         BAD_CAST format));
-                                           BAD_CAST drv_priv->format));
-  }
-  else if (drv_priv->real_src.protocol == drive_protocol_file) {
-    /* libvirt has disabled the feature of detecting the disk format,
-     * unless the administrator sets allow_disk_format_probing=1 in
-     * qemu.conf.  There is no way to detect if this option is set, so we
-     * have to do format detection here using qemu-img and pass that to
-     * libvirt.
-     *
-     * This is still a security issue, so in most cases it is recommended
-     * the users pass the format to libguestfs which will faithfully pass
-     * that to libvirt and this function won't be used.
-     */
-    format = guestfs_disk_format (g, drv_priv->real_src.u.path);
-    if (!format)
-      return -1;
-
-    if (STREQ (format, "unknown")) {
-      error (g, _("could not auto-detect the format.\n"
-                  "If the format is known, pass the format to libguestfs, eg. using the\n"
-                  "'--format' option, or via the optional 'format' argument to 'add-drive'."));
-      return -1;
-    }
-
-    XMLERROR (-1,
-              xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-                                           BAD_CAST format));
-  }
-  else {
-    error (g, _("could not auto-detect the format when using a non-file protocol.\n"
-                "If the format is known, pass the format to libguestfs, eg. using the\n"
-                "'--format' option, or via the optional 'format' argument to 'add-drive'."));
-    return -1;
-  }
-
   XMLERROR (-1,
             xmlTextWriterWriteAttribute (xo, BAD_CAST "cache",
-                                         BAD_CAST (drv->cachemode ?
+                                         BAD_CAST cachemode));
-                                                   drv->cachemode :
-                                                   "writeback")));
-
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
-  if (drv->disk_label) {
+  return 0;
-    XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "serial"));
+}
-    XMLERROR (-1, xmlTextWriterWriteString (xo, BAD_CAST drv->disk_label));
+
-    XMLERROR (-1, xmlTextWriterEndElement (xo));
+static int
-  }
+construct_libvirt_xml_disk_address (guestfs_h *g, xmlTextWriterPtr xo,
+                                    size_t drv_index)
+{
+  char scsi_target[64];
+
+  snprintf (scsi_target, sizeof scsi_target, "%zu", drv_index);
 
   XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "address"));
   XMLERROR (-1,
@@ -1318,8 +1435,6 @@ construct_libvirt_xml_disk (guestfs_h *g,
                                          BAD_CAST "0"));
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
-  XMLERROR (-1, xmlTextWriterEndElement (xo));
-
   return 0;
 }
 
@@ -1398,10 +1513,6 @@ construct_libvirt_xml_appliance (guestfs_h *g,
                                  const struct libvirt_xml_params *params,
                                  xmlTextWriterPtr xo)
 {
-  char scsi_target[64];
-
-  snprintf (scsi_target, sizeof scsi_target, "%zu", params->appliance_index);
-
   XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "disk"));
   XMLERROR (-1,
             xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
@@ -1425,46 +1536,15 @@ construct_libvirt_xml_appliance (guestfs_h *g,
                                          BAD_CAST "scsi"));
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
-  XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "driver"));
+  if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2", "unsafe") == -1)
-  XMLERROR (-1,
+    return -1;
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "name",
-                                         BAD_CAST "qemu"));
-  XMLERROR (-1,
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-                                         BAD_CAST "qcow2"));
-  XMLERROR (-1,
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "cache",
-                                         BAD_CAST "unsafe"));
-  XMLERROR (-1, xmlTextWriterEndElement (xo));
 
-  XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "address"));
+  if (construct_libvirt_xml_disk_address (g, xo, params->appliance_index) == -1)
-  XMLERROR (-1,
+    return -1;
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "type",
-                                         BAD_CAST "drive"));
-  XMLERROR (-1,
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "controller",
-                                         BAD_CAST "0"));
-  XMLERROR (-1,
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "bus",
-                                         BAD_CAST "0"));
-  XMLERROR (-1,
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "target",
-                                         BAD_CAST scsi_target));
-  XMLERROR (-1,
-            xmlTextWriterWriteAttribute (xo, BAD_CAST "unit",
-                                         BAD_CAST "0"));
-  XMLERROR (-1, xmlTextWriterEndElement (xo));
 
   XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "shareable"));
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
-  /* We'd like to do this, but it's not supported by libvirt.
-   * See make_drive_priv for the workaround.
-   *
-   * XMLERROR (-1, xmlTextWriterStartElement (xo, BAD_CAST "transient"));
-   * XMLERROR (-1, xmlTextWriterEndElement (xo));
-   */
-
   XMLERROR (-1, xmlTextWriterEndElement (xo));
 
   return 0;
@@ -1562,143 +1642,6 @@ ignore_errors (void *ignore, virErrorPtr ignore2)
   /* empty */
 }
 
-/* Create a temporary qcow2 overlay on top of 'backing_device', which is
- * either an absolute path or a qemu device name.
- */
-static char *
-make_qcow2_overlay (guestfs_h *g, const char *backing_device,
-                    const char *format, const char *selinux_imagelabel)
-{
-  char *tmpfile = NULL;
-  CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g);
-  int r;
-
-  tmpfile = safe_asprintf (g, "%s/snapshot%d", g->tmpdir, ++g->unique);
-
-  guestfs___cmd_add_arg (cmd, "qemu-img");
-  guestfs___cmd_add_arg (cmd, "create");
-  guestfs___cmd_add_arg (cmd, "-f");
-  guestfs___cmd_add_arg (cmd, "qcow2");
-  guestfs___cmd_add_arg (cmd, "-b");
-  guestfs___cmd_add_arg (cmd, backing_device);
-  if (format) {
-    guestfs___cmd_add_arg (cmd, "-o");
-    guestfs___cmd_add_arg_format (cmd, "backing_fmt=%s", format);
-  }
-  guestfs___cmd_add_arg (cmd, tmpfile);
-  r = guestfs___cmd_run (cmd);
-  if (r == -1)
-    goto error;
-  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
-    guestfs___external_command_failed (g, r, "qemu-img create", backing_device);
-    goto error;
-  }
-
-#if HAVE_LIBSELINUX
-  if (selinux_imagelabel) {
-    debug (g, "setting SELinux label on %s to %s",
-           tmpfile, selinux_imagelabel);
-    if (setfilecon (tmpfile, (security_context_t) selinux_imagelabel) == -1)
-      selinux_warning (g, __func__, "setfilecon", tmpfile);
-  }
-#endif
-
-  return tmpfile;               /* caller frees */
-
- error:
-  free (tmpfile);
-
-  return NULL;
-}
-
-/* This sets up the drv->priv structure, which contains the drive
- * that we're actually going to add.  If asked to make a drive readonly
- * then because libvirt doesn't support <transient/> we have to add
- * a qcow2 overlay here.
- */
-static int
-make_drive_priv (guestfs_h *g, struct drive *drv,
-                 const char *selinux_imagelabel)
-{
-  char *path;
-  struct drive_libvirt *drv_priv;
-
-  if (drv->priv && drv->free_priv)
-    drv->free_priv (drv->priv);
-
-  drv->priv = drv_priv = safe_calloc (g, 1, sizeof (struct drive_libvirt));
-  drv->free_priv = drive_free_priv;
-
-  switch (drv->src.protocol) {
-  case drive_protocol_file:
-
-    /* Even for non-readonly paths, we need to make the paths absolute here. */
-    path = realpath (drv->src.u.path, NULL);
-    if (path == NULL) {
-      perrorf (g, _("realpath: could not convert '%s' to absolute path"),
-               drv->src.u.path);
-      return -1;
-    }
-
-    drv_priv->real_src.protocol = drive_protocol_file;
-
-    if (!drv->readonly) {
-      drv_priv->real_src.u.path = path;
-      drv_priv->format = drv->format ? safe_strdup (g, drv->format) : NULL;
-    }
-    else {
-      drv_priv->real_src.u.path = make_qcow2_overlay (g, path, drv->format,
-                                                      selinux_imagelabel);
-      free (path);
-      if (!drv_priv->real_src.u.path)
-        return -1;
-      drv_priv->format = safe_strdup (g, "qcow2");
-    }
-    break;
-
-  case drive_protocol_ftp:
-  case drive_protocol_ftps:
-  case drive_protocol_gluster:
-  case drive_protocol_http:
-  case drive_protocol_https:
-  case drive_protocol_iscsi:
-  case drive_protocol_nbd:
-  case drive_protocol_rbd:
-  case drive_protocol_sheepdog:
-  case drive_protocol_ssh:
-  case drive_protocol_tftp:
-    if (!drv->readonly) {
-      guestfs___copy_drive_source (g, &drv->src, &drv_priv->real_src);
-      drv_priv->format = drv->format ? safe_strdup (g, drv->format) : NULL;
-    }
-    else {
-      CLEANUP_FREE char *qemu_device = NULL;
-
-      drv_priv->real_src.protocol = drive_protocol_file;
-      qemu_device = guestfs___drive_source_qemu_param (g, &drv->src);
-      drv_priv->real_src.u.path = make_qcow2_overlay (g, qemu_device,
-                                                      drv->format,
-                                                      selinux_imagelabel);
-      if (!drv_priv->real_src.u.path)
-        return -1;
-      drv_priv->format = safe_strdup (g, "qcow2");
-    }
-    break;
-  }
-
-  return 0;
-}
-
-static void
-drive_free_priv (void *priv)
-{
-  struct drive_libvirt *drv_priv = priv;
-
-  guestfs___free_drive_source (&drv_priv->real_src);
-  free (drv_priv->format);
-  free (drv_priv);
-}
-
 static int
 shutdown_libvirt (guestfs_h *g, void *datav, int check_for_errors)
 {
@@ -1805,9 +1748,6 @@ hot_add_drive_libvirt (guestfs_h *g, void *datav,
     return -1;
   }
 
-  if (make_drive_priv (g, drv, data->selinux_imagelabel) == -1)
-    return -1;
-
   /* Create the XML for the new disk. */
   xml = construct_libvirt_xml_hot_add_disk (g, data, drv, drv_index);
   if (xml == NULL)
@@ -1908,6 +1848,7 @@ set_libvirt_selinux_norelabel_disks (guestfs_h *g, void *datav, int flag)
 
 static struct backend_ops backend_libvirt_ops = {
   .data_size = sizeof (struct backend_libvirt_data),
+  .create_cow_overlay = create_cow_overlay_libvirt,
   .launch = launch_libvirt,
   .shutdown = shutdown_libvirt,
   .max_disks = max_disks_libvirt,

src/launch-uml.c

@@ -50,7 +50,42 @@ struct backend_uml_data {
 };
 
 static void print_vmlinux_command_line (guestfs_h *g, char **argv);
-static char *make_cow_overlay (guestfs_h *g, const char *original);
+
+/* Run uml_mkcow to create a COW overlay. */
+static char *
+make_cow_overlay (guestfs_h *g, const char *original)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g);
+  char *overlay;
+  int r;
+
+  if (guestfs___lazy_make_tmpdir (g) == -1)
+    return NULL;
+
+  overlay = safe_asprintf (g, "%s/overlay%d", g->tmpdir, g->unique++);
+
+  guestfs___cmd_add_arg (cmd, "uml_mkcow");
+  guestfs___cmd_add_arg (cmd, overlay);
+  guestfs___cmd_add_arg (cmd, original);
+  r = guestfs___cmd_run (cmd);
+  if (r == -1) {
+    free (overlay);
+    return NULL;
+  }
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    guestfs___external_command_failed (g, r, "uml_mkcow", original);
+    free (overlay);
+    return NULL;
+  }
+
+  return overlay;
+}
+
+static char *
+create_cow_overlay_uml (guestfs_h *g, void *datav, struct drive *drv)
+{
+  return make_cow_overlay (g, drv->src.u.path);
+}
 
 /* Test for features which are not supported by the UML backend.
  * Possibly some of these should just be warnings, not errors.
@@ -75,7 +110,7 @@ uml_supported (guestfs_h *g)
       error (g, _("uml backend does not support remote drives"));
       return false;
     }
-    if (drv->format && STRNEQ (drv->format, "raw")) {
+    if (drv->src.format && STRNEQ (drv->src.format, "raw")) {
       error (g, _("uml backend does not support non-raw-format drives"));
       return false;
     }
@@ -132,20 +167,10 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
     return -1;
   has_appliance_drive = appliance != NULL;
 
-  /* Create COW overlays for any readonly drives, and for the root.
+  /* Create COW overlays for the appliance.  Note that the documented
-   * Note that the documented syntax ubd0=cow,orig does not work since
+   * syntax ubd0=cow,orig does not work since kernel 3.3.  See:
-   * kernel 3.3.  See:
    * http://thread.gmane.org/gmane.linux.uml.devel/13556
    */
-  ITER_DRIVES (g, i, drv) {
-    if (drv->readonly) {
-      drv->priv = make_cow_overlay (g, drv->src.u.path);
-      if (!drv->priv)
-        goto cleanup0;
-      drv->free_priv = free;
-    }
-  }
-
   if (has_appliance_drive) {
     appliance_cow = make_cow_overlay (g, appliance);
     if (!appliance_cow)
@@ -219,10 +244,10 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
 
   /* Add the drives. */
   ITER_DRIVES (g, i, drv) {
-    if (!drv->readonly)
+    if (!drv->overlay)
       ADD_CMDLINE_PRINTF ("ubd%zu=%s", i, drv->src.u.path);
     else
-      ADD_CMDLINE_PRINTF ("ubd%zu=%s", i, (char *) drv->priv);
+      ADD_CMDLINE_PRINTF ("ubd%zu=%s", i, drv->overlay);
   }
 
   /* Add the ext2 appliance drive (after all the drives). */
@@ -479,35 +504,6 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
   return -1;
 }
 
-/* Run uml_mkcow to create a COW overlay.  This works around a kernel
- * bug in UML option parsing.
- */
-static char *
-make_cow_overlay (guestfs_h *g, const char *original)
-{
-  CLEANUP_CMD_CLOSE struct command *cmd = guestfs___new_command (g);
-  char *cow;
-  int r;
-
-  cow = safe_asprintf (g, "%s/cow%d", g->tmpdir, g->unique++);
-
-  guestfs___cmd_add_arg (cmd, "uml_mkcow");
-  guestfs___cmd_add_arg (cmd, cow);
-  guestfs___cmd_add_arg (cmd, original);
-  r = guestfs___cmd_run (cmd);
-  if (r == -1) {
-    free (cow);
-    return NULL;
-  }
-  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
-    guestfs___external_command_failed (g, r, "uml_mkcow", original);
-    free (cow);
-    return NULL;
-  }
-
-  return cow;                   /* caller must free */
-}
-
 /* This is called from the forked subprocess just before vmlinux 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.
@@ -605,6 +601,7 @@ max_disks_uml (guestfs_h *g, void *datav)
 
 static struct backend_ops backend_uml_ops = {
   .data_size = sizeof (struct backend_uml_data),
+  .create_cow_overlay = create_cow_overlay_uml,
   .launch = launch_uml,
   .shutdown = shutdown_uml,
   .get_pid = get_pid_uml,

src/libvirt-domain.c

@@ -235,7 +235,10 @@ guestfs___add_libvirt_dom (guestfs_h *g, virDomainPtr dom,
   if ((doc = get_domain_xml (g, dom)) == NULL)
     return -1;
 
-  /* Find and pass the SELinux security label to the libvirt back end. */
+  /* Find and pass the SELinux security label to the libvirt back end.
+   * Note this has to happen before adding the disks, since those may
+   * use the label.
+   */
   if (libvirt_selinux_label (g, doc, &label, &imagelabel) == -1)
     return -1;
   if (label && imagelabel) {