Libguestfs - May 2016 - [PATCH 0/4] lib: qemu: Memoize qemu feature detection.

Doing qemu feature detection in the direct backend takes ~100ms
because we need to run `qemu -help' and `qemu -devices ?', and each of
those interacts with glibc's very slow link loader.

Fixing the link loader is really hard.  Instead memoize the
output of those two commands.

This patch series first separates all the code dealing with qemu into
a separate module (src/qemu.c) and formalizes the interface to this
module.  Then I had to rearrange the code we use to create
/var/tmp/.guestfs-<UID> for the supermin appliance to src/tmpdirs.c.
The final patch adds the memoization, storing this in the supermin
appliance cache directory.

Rich.

This is code motion, but I have cleaned up and formalized the
interface between this module and other parts of the library.

Also this adds documentation to the interface.
---
 docs/C_SOURCE_FILES    |   1 +
 src/Makefile.am        |   1 +
 src/guestfs-internal.h |  19 +-
 src/launch-direct.c    | 586 +++-------------------------------------------
 src/launch-libvirt.c   |  13 +-
 src/qemu.c             | 619 +++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 674 insertions(+), 565 deletions(-)
 create mode 100644 src/qemu.c

diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES
index 10e02de..4f1f684 100644
--- a/docs/C_SOURCE_FILES
+++ b/docs/C_SOURCE_FILES
@@ -253,6 +253,7 @@ src/mountable.c
 src/osinfo.c
 src/private-data.c
 src/proto.c
+src/qemu.c
 src/stringsbuf.c
 src/structs-cleanup.c
 src/structs-compare.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 7a694ca..f7badad 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -125,6 +125,7 @@ libguestfs_la_SOURCES = \
 	osinfo.c \
 	private-data.c \
 	proto.c \
+	qemu.c \
 	stringsbuf.c \
 	structs-compare.c \
 	structs-copy.c \
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index fab9172..dbd0a98 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -900,11 +900,6 @@ extern char *guestfs_int_cmd_get_pipe_errors (struct
command *cmd);
 #endif
 extern void guestfs_int_cleanup_cmd_close (struct command **);
 
-/* launch-direct.c */
-extern char *guestfs_int_drive_source_qemu_param (guestfs_h *g, const struct
drive_source *src);
-extern bool guestfs_int_discard_possible (guestfs_h *g, struct drive *drv,
unsigned long qemu_version);
-extern char *guestfs_int_qemu_escape_param (guestfs_h *g, const char *param);
-
 /* launch-*.c constructors */
 void guestfs_int_init_direct_backend (void) __attribute__((constructor));
 #ifdef HAVE_LIBVIRT_BACKEND
@@ -913,6 +908,20 @@ void guestfs_int_init_libvirt_backend (void)
__attribute__((constructor));
 void guestfs_int_init_uml_backend (void) __attribute__((constructor));
 void guestfs_int_init_unix_backend (void) __attribute__((constructor));
 
+/* qemu.c */
+struct qemu_data;
+extern struct qemu_data *guestfs_int_test_qemu (guestfs_h *g);
+extern struct qemu_data *guestfs_int_init_qemu_version (guestfs_h *g, unsigned
long qemu_version);
+extern int guestfs_int_qemu_supports (guestfs_h *g, const struct qemu_data *,
const char *option);
+extern int guestfs_int_qemu_supports_device (guestfs_h *g, const struct
qemu_data *, const char *device_name);
+extern int guestfs_int_qemu_supports_virtio_scsi (guestfs_h *g, struct
qemu_data *);
+extern bool guestfs_int_qemu_version_le (const struct qemu_data *, int
max_major, int max_minor);
+extern bool guestfs_int_qemu_version_ge (const struct qemu_data *, int
min_major, int min_minor);
+extern char *guestfs_int_drive_source_qemu_param (guestfs_h *g, const struct
drive_source *src);
+extern bool guestfs_int_discard_possible (guestfs_h *g, struct drive *drv,
const struct qemu_data *);
+extern char *guestfs_int_qemu_escape_param (guestfs_h *g, const char *param);
+extern void guestfs_int_free_qemu_data (struct qemu_data *);
+
 /* guid.c */
 extern int guestfs_int_validate_guid (const char *);
 
diff --git a/src/launch-direct.c b/src/launch-direct.c
index 64f0de6..c8a33c8 100644
--- a/src/launch-direct.c
+++ b/src/launch-direct.c
@@ -40,31 +40,18 @@
 #include <string.h>
 #include <libintl.h>
 
-#include <pcre.h>
-
-#include <libxml/uri.h>
-
 #include "cloexec.h"
-#include "ignore-value.h"
 
 #include "guestfs.h"
 #include "guestfs-internal.h"
 #include "guestfs_protocol.h"
 
-COMPILE_REGEXP (re_major_minor, "(\\d+)\\.(\\d+)", 0)
-
 /* Per-handle data. */
 struct backend_direct_data {
-  pid_t pid;                  /* Qemu PID. */
-  pid_t recoverypid;          /* Recovery process PID. */
+  pid_t pid;                    /* Qemu PID. */
+  pid_t recoverypid;            /* Recovery process PID. */
 
-  char *qemu_help;            /* Output of qemu -help. */
-  char *qemu_devices;         /* Output of qemu -device ? */
-
-  /* qemu version (0, 0 if unable to parse). */
-  int qemu_version_major, qemu_version_minor;
-
-  int virtio_scsi;        /* See function qemu_supports_virtio_scsi */
+  struct qemu_data *qemu_data;  /* qemu version etc. */
 
   char guestfsd_sock[UNIX_PATH_MAX]; /* Path to daemon socket. */
 };
@@ -72,9 +59,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, int virtio_scsi);
 static void print_qemu_command_line (guestfs_h *g, char **argv);
-static int qemu_supports (guestfs_h *g, struct backend_direct_data *, const
char *option);
-static int qemu_supports_device (guestfs_h *g, struct backend_direct_data *,
const char *device_name);
-static int qemu_supports_virtio_scsi (guestfs_h *g, struct backend_direct_data
*);
 
 static char *
 create_cow_overlay_direct (guestfs_h *g, void *datav, struct drive *drv)
@@ -291,7 +275,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   debug (g, "begin testing qemu features");
 
   /* Get qemu help text and version. */
-  if (qemu_supports (g, data, NULL) == -1)
+  data->qemu_data = guestfs_int_test_qemu (g);
+  if (data->qemu_data == NULL)
     goto cleanup0;
 
   /* Using virtio-serial, we need to create a local Unix domain socket
@@ -349,19 +334,19 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
    * strings to it so we don't need to check for the specific virtio
    * feature.
    */
-  if (qemu_supports (g, data, "-global")) {
+  if (guestfs_int_qemu_supports (g, data->qemu_data, "-global")) {
     ADD_CMDLINE ("-global");
     ADD_CMDLINE (VIRTIO_BLK ".scsi=off");
   }
 
-  if (qemu_supports (g, data, "-nodefconfig"))
+  if (guestfs_int_qemu_supports (g, data->qemu_data,
"-nodefconfig"))
     ADD_CMDLINE ("-nodefconfig");
 
   /* This oddly named option doesn't actually enable FIPS.  It just
    * causes qemu to do the right thing if FIPS is enabled in the
    * kernel.  So like libvirt, we pass it unconditionally.
    */
-  if (qemu_supports (g, data, "-enable-fips"))
+  if (guestfs_int_qemu_supports (g, data->qemu_data,
"-enable-fips"))
     ADD_CMDLINE ("-enable-fips");
 
   /* Newer versions of qemu (from around 2009/12) changed the
@@ -372,7 +357,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
    * called -nodefaults which gets rid of all this default crud, so
    * let's use that to avoid this and any future surprises.
    */
-  if (qemu_supports (g, data, "-nodefaults"))
+  if (guestfs_int_qemu_supports (g, data->qemu_data,
"-nodefaults"))
     ADD_CMDLINE ("-nodefaults");
 
   /* This disables the host-side display (SDL, Gtk). */
@@ -426,11 +411,10 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   /* These are recommended settings, see RHBZ#1053847. */
   ADD_CMDLINE ("-rtc");
   ADD_CMDLINE ("driftfix=slew");
-  if (qemu_supports (g, data, "-no-hpet")) {
+  if (guestfs_int_qemu_supports (g, data->qemu_data, "-no-hpet"))
{
     ADD_CMDLINE ("-no-hpet");
   }
-  if (data->qemu_version_major < 1 ||
-      (data->qemu_version_major == 1 && data->qemu_version_minor
<= 2))
+  if (guestfs_int_qemu_version_le (data->qemu_data, 1, 2))
     ADD_CMDLINE ("-no-kvm-pit-reinjection");
   else {
     /* New non-deprecated way, added in qemu >= 1.3. */
@@ -460,7 +444,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
    * isn't strictly necessary but means we won't need to hang around
    * when needing entropy.
    */
-  if (qemu_supports_device (g, data, "virtio-rng-pci")) {
+  if (guestfs_int_qemu_supports_device (g, data->qemu_data,
+                                        "virtio-rng-pci")) {
     ADD_CMDLINE ("-object");
     ADD_CMDLINE ("rng-random,filename=/dev/urandom,id=rng0");
     ADD_CMDLINE ("-device");
@@ -468,7 +453,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   }
 
   /* Add drives */
-  virtio_scsi = qemu_supports_virtio_scsi (g, data);
+  virtio_scsi = guestfs_int_qemu_supports_virtio_scsi (g, data->qemu_data);
 
   if (virtio_scsi) {
     /* Create the virtio-scsi bus. */
@@ -481,8 +466,6 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
 
     if (!drv->overlay) {
       const char *discard_mode = "";
-      int major = data->qemu_version_major, minor =
data->qemu_version_minor;
-      unsigned long qemu_version = major * 1000000 + minor * 1000;
 
       switch (drv->discard) {
       case discard_disable:
@@ -492,14 +475,14 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
          */
         break;
       case discard_enable:
-        if (!guestfs_int_discard_possible (g, drv, qemu_version))
+        if (!guestfs_int_discard_possible (g, drv, data->qemu_data))
           goto cleanup0;
         /*FALLTHROUGH*/
       case discard_besteffort:
         /* I believe from reading the code that this is always safe as
          * long as qemu >= 1.5.
          */
-        if (major > 1 || (major == 1 && minor >= 5))
+        if (guestfs_int_qemu_version_ge (data->qemu_data, 1, 5))
           discard_mode = ",discard=unmap";
         break;
       }
@@ -592,7 +575,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   ADD_CMDLINE ("stdio");
 
   if (g->verbose &&
-      qemu_supports_device (g, data, "Serial Graphics Adapter")) {
+      guestfs_int_qemu_supports_device (g, data->qemu_data,
+                                        "Serial Graphics Adapter")) {
     /* Use sgabios instead of vgabios.  This means we'll see BIOS
      * messages on the serial port, and also works around this bug
      * in qemu 1.1.0:
@@ -860,6 +844,8 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   data->pid = 0;
   data->recoverypid = 0;
   memset (&g->launch_t, 0, sizeof g->launch_t);
+  guestfs_int_free_qemu_data (data->qemu_data);
+  data->qemu_data = NULL;
 
  cleanup0:
   if (daemon_accept_sock >= 0)
@@ -941,150 +927,6 @@ print_qemu_command_line (guestfs_h *g, char **argv)
   fputc ('\n', stderr);
 }
 
-static void parse_qemu_version (guestfs_h *g, struct backend_direct_data
*data);
-static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
-
-/* Test qemu binary (or wrapper) runs, and do 'qemu -help' so we know
- * the version of qemu what options this qemu supports.
- */
-static int
-test_qemu (guestfs_h *g, struct backend_direct_data *data)
-{
-  CLEANUP_CMD_CLOSE struct command *cmd1 = guestfs_int_new_command (g);
-  CLEANUP_CMD_CLOSE struct command *cmd2 = guestfs_int_new_command (g);
-  int r;
-
-  free (data->qemu_help);
-  data->qemu_help = NULL;
-  free (data->qemu_devices);
-  data->qemu_devices = NULL;
-
-  guestfs_int_cmd_add_arg (cmd1, g->hv);
-  guestfs_int_cmd_add_arg (cmd1, "-display");
-  guestfs_int_cmd_add_arg (cmd1, "none");
-  guestfs_int_cmd_add_arg (cmd1, "-help");
-  guestfs_int_cmd_set_stdout_callback (cmd1, read_all, &data->qemu_help,
-				       CMD_STDOUT_FLAG_WHOLE_BUFFER);
-  r = guestfs_int_cmd_run (cmd1);
-  if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0)
-    goto error;
-
-  parse_qemu_version (g, data);
-
-  guestfs_int_cmd_add_arg (cmd2, g->hv);
-  guestfs_int_cmd_add_arg (cmd2, "-display");
-  guestfs_int_cmd_add_arg (cmd2, "none");
-  guestfs_int_cmd_add_arg (cmd2, "-machine");
-  guestfs_int_cmd_add_arg (cmd2,
-#ifdef MACHINE_TYPE
-                           MACHINE_TYPE ","
-#endif
-                           "accel=kvm:tcg");
-  guestfs_int_cmd_add_arg (cmd2, "-device");
-  guestfs_int_cmd_add_arg (cmd2, "?");
-  guestfs_int_cmd_clear_capture_errors (cmd2);
-  guestfs_int_cmd_set_stderr_to_stdout (cmd2);
-  guestfs_int_cmd_set_stdout_callback (cmd2, read_all,
&data->qemu_devices,
-				       CMD_STDOUT_FLAG_WHOLE_BUFFER);
-  r = guestfs_int_cmd_run (cmd2);
-  if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0)
-    goto error;
-
-  return 0;
-
- error:
-  if (r == -1)
-    return -1;
-
-  guestfs_int_external_command_failed (g, r, g->hv, NULL);
-  return -1;
-}
-
-/* Parse the first line of data->qemu_help (if not NULL) into the
- * major and minor version of qemu, but don't fail if parsing is not
- * possible.
- */
-static void
-parse_qemu_version (guestfs_h *g, struct backend_direct_data *data)
-{
-  CLEANUP_FREE char *major_s = NULL, *minor_s = NULL;
-  int major_i, minor_i;
-
-  data->qemu_version_major = 0;
-  data->qemu_version_minor = 0;
-
-  if (!data->qemu_help)
-    return;
-
-  if (!match2 (g, data->qemu_help, re_major_minor, &major_s,
&minor_s)) {
-  parse_failed:
-    debug (g, "%s: failed to parse qemu version string from the first line
of the output of '%s -help'.  When reporting this bug please include the
-help output.",
-           __func__, g->hv);
-    return;
-  }
-
-  major_i = guestfs_int_parse_unsigned_int (g, major_s);
-  if (major_i == -1)
-    goto parse_failed;
-
-  minor_i = guestfs_int_parse_unsigned_int (g, minor_s);
-  if (minor_i == -1)
-    goto parse_failed;
-
-  data->qemu_version_major = major_i;
-  data->qemu_version_minor = minor_i;
-
-  debug (g, "qemu version %d.%d", major_i, minor_i);
-}
-
-static void
-read_all (guestfs_h *g, void *retv, const char *buf, size_t len)
-{
-  char **ret = retv;
-
-  *ret = safe_strndup (g, buf, len);
-}
-
-/* Test if option is supported by qemu command line (just by grepping
- * the help text).
- *
- * The first time this is used, it has to run the external qemu
- * binary.  If that fails, it returns -1.
- *
- * To just do the first-time run of the qemu binary, call this with
- * option == NULL, in which case it will return -1 if there was an
- * error doing that.
- */
-static int
-qemu_supports (guestfs_h *g, struct backend_direct_data *data,
-               const char *option)
-{
-  if (!data->qemu_help) {
-    if (test_qemu (g, data) == -1)
-      return -1;
-  }
-
-  if (option == NULL)
-    return 1;
-
-  return strstr (data->qemu_help, option) != NULL;
-}
-
-/* Test if device is supported by qemu (currently just greps the -device ?
- * output).
- */
-static int
-qemu_supports_device (guestfs_h *g, struct backend_direct_data *data,
-                      const char *device_name)
-{
-  if (!data->qemu_devices) {
-    if (test_qemu (g, data) == -1)
-      return -1;
-  }
-
-  return strstr (data->qemu_devices, device_name) != NULL;
-}
-
 /* Check if a file can be opened. */
 static int
 is_openable (guestfs_h *g, const char *path, int flags)
@@ -1099,382 +941,6 @@ is_openable (guestfs_h *g, const char *path, int flags)
 }
 
 static int
-old_or_broken_virtio_scsi (guestfs_h *g, struct backend_direct_data *data)
-{
-  /* qemu 1.1 claims to support virtio-scsi but in reality it's broken. */
-  if (data->qemu_version_major == 1 && data->qemu_version_minor
< 2)
-    return 1;
-
-  return 0;
-}
-
-/* Returns 1 = use virtio-scsi, or 0 = use virtio-blk. */
-static int
-qemu_supports_virtio_scsi (guestfs_h *g, struct backend_direct_data *data)
-{
-  int r;
-
-  if (!data->qemu_help) {
-    if (test_qemu (g, data) == -1)
-      return 0;                 /* safe option? */
-  }
-
-  /* data->virtio_scsi has these values:
-   *   0 = untested (after handle creation)
-   *   1 = supported
-   *   2 = not supported (use virtio-blk)
-   *   3 = test failed (use virtio-blk)
-   */
-  if (data->virtio_scsi == 0) {
-    if (old_or_broken_virtio_scsi (g, data))
-      data->virtio_scsi = 2;
-    else {
-      r = qemu_supports_device (g, data, VIRTIO_SCSI);
-      if (r > 0)
-        data->virtio_scsi = 1;
-      else if (r == 0)
-        data->virtio_scsi = 2;
-      else
-        data->virtio_scsi = 3;
-    }
-  }
-
-  return data->virtio_scsi == 1;
-}
-
-/**
- * Escape a qemu parameter.
- *
- * Every C<,> becomes C<,,>.  The caller must free the returned
string.
- */
-char *
-guestfs_int_qemu_escape_param (guestfs_h *g, const char *param)
-{
-  size_t i, len = strlen (param);
-  char *p, *ret;
-
-  ret = p = safe_malloc (g, len*2 + 1); /* max length of escaped name*/
-  for (i = 0; i < len; ++i) {
-    *p++ = param[i];
-    if (param[i] == ',')
-      *p++ = ',';
-  }
-  *p = '\0';
-
-  return ret;
-}
-
-static char *
-make_uri (guestfs_h *g, const char *scheme, const char *user,
-          const char *password,
-          struct drive_server *server, const char *path)
-{
-  xmlURI uri = { .scheme = (char *) scheme,
-                 .user = (char *) user };
-  CLEANUP_FREE char *query = NULL;
-  CLEANUP_FREE char *pathslash = NULL;
-  CLEANUP_FREE char *userauth = NULL;
-
-  /* Need to add a leading '/' to URI paths since xmlSaveUri
doesn't. */
-  if (path != NULL && path[0] != '/') {
-    pathslash = safe_asprintf (g, "/%s", path);
-    uri.path = pathslash;
-  }
-  else
-    uri.path = (char *) path;
-
-  /* Rebuild user:password. */
-  if (user != NULL && password != NULL) {
-    /* Keep the string in an own variable so it can be freed automatically. */
-    userauth = safe_asprintf (g, "%s:%s", user, password);
-    uri.user = userauth;
-  }
-
-  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_int_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->secret,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_ftps:
-    return make_uri (g, "ftps", src->username, src->secret,
-                     &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, NULL,
-                       &src->servers[0], src->u.exportname);
-    case drive_transport_tcp:
-      return make_uri (g, "gluster+tcp", NULL, NULL,
-                       &src->servers[0], src->u.exportname);
-    case drive_transport_unix:
-      return make_uri (g, "gluster+unix", NULL, NULL,
-                       &src->servers[0], NULL);
-    }
-
-  case drive_protocol_http:
-    return make_uri (g, "http", src->username, src->secret,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_https:
-    return make_uri (g, "https", src->username, src->secret,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_iscsi: {
-    CLEANUP_FREE char *escaped_hostname = NULL;
-    CLEANUP_FREE char *escaped_target = NULL;
-    CLEANUP_FREE char *userauth = NULL;
-    char port_str[16];
-    char *ret;
-
-    escaped_hostname -      (char *) xmlURIEscapeStr(BAD_CAST
src->servers[0].u.hostname,
-                               BAD_CAST "");
-    /* The target string must keep slash as it is, as exportname contains
-     * "iqn/lun".
-     */
-    escaped_target -      (char *) xmlURIEscapeStr(BAD_CAST
src->u.exportname, BAD_CAST "/");
-    if (src->username != NULL && src->secret != NULL)
-      userauth = safe_asprintf (g, "%s%%%s@", src->username,
src->secret);
-    if (src->servers[0].port != 0)
-      snprintf (port_str, sizeof port_str, ":%d",
src->servers[0].port);
-
-    ret = safe_asprintf (g, "iscsi://%s%s%s/%s",
-                         userauth != NULL ? userauth : "",
-                         escaped_hostname,
-                         src->servers[0].port != 0 ? port_str :
"",
-                         escaped_target);
-
-    return ret;
-  }
-
-  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
-      ret = safe_asprintf (g, "%s:exportname=%s", p,
src->u.exportname);
-
-    return ret;
-  }
-
-  case drive_protocol_rbd: {
-    CLEANUP_FREE char *mon_host = NULL, *username = NULL, *secret = NULL;
-    const char *auth;
-    size_t n = 0;
-    size_t i, j;
-
-    /* build the list of all the mon hosts */
-    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";
-
-    return safe_asprintf (g, "rbd:%s%s%s%s%s%s",
-                          src->u.exportname,
-                          src->nr_servers > 0 ? ":mon_host=" :
"",
-                          src->nr_servers > 0 ? mon_host : "",
-                          username ? username : "",
-                          auth,
-                          secret ? secret : "");
-  }
-
-  case drive_protocol_sheepdog:
-    if (src->nr_servers == 0)
-      return safe_asprintf (g, "sheepdog:%s", src->u.exportname);
-    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);
-
-  case drive_protocol_ssh:
-    return make_uri (g, "ssh", src->username, src->secret,
-                     &src->servers[0], src->u.exportname);
-
-  case drive_protocol_tftp:
-    return make_uri (g, "tftp", src->username, src->secret,
-                     &src->servers[0], src->u.exportname);
-  }
-
-  abort ();
-}
-
-/* Test if discard is both supported by qemu AND possible with the
- * underlying file or device.  This returns 1 if discard is possible.
- * It returns 0 if not possible and sets the error to the reason why.
- *
- * This function is called when the user set discard == "enable".
- *
- * qemu_version is the version of qemu in the form returned by libvirt:
- * major * 1,000,000 + minor * 1,000 + release
- */
-bool
-guestfs_int_discard_possible (guestfs_h *g, struct drive *drv,
-			      unsigned long qemu_version)
-{
-  /* qemu >= 1.5.  This was the first version that supported the
-   * discard option on -drive at all.
-   */
-  bool qemu15 = qemu_version >= 1005000;
-  /* qemu >= 1.6.  This was the first version that supported unmap on
-   * qcow2 backing files.
-   */
-  bool qemu16 = qemu_version >= 1006000;
-
-  if (!qemu15)
-    NOT_SUPPORTED (g, false,
-                   _("discard cannot be enabled on this drive: "
-                     "qemu < 1.5"));
-
-  /* If it's an overlay, discard is not possible (on the underlying
-   * file).  This has probably been caught earlier since we already
-   * checked that the drive is !readonly.  Nevertheless ...
-   */
-  if (drv->overlay)
-    NOT_SUPPORTED (g, false,
-                   _("discard cannot be enabled on this drive: "
-                     "the drive has a read-only overlay"));
-
-  /* Look at the source format. */
-  if (drv->src.format == NULL) {
-    /* We could autodetect the format, but we don't ... yet. XXX */
-    NOT_SUPPORTED (g, false,
-                   _("discard cannot be enabled on this drive: "
-                     "you have to specify the format of the file"));
-  }
-  else if (STREQ (drv->src.format, "raw"))
-    /* OK */ ;
-  else if (STREQ (drv->src.format, "qcow2")) {
-    if (!qemu16)
-      NOT_SUPPORTED (g, false,
-                     _("discard cannot be enabled on this drive: "
-                       "qemu < 1.6 cannot do discard on qcow2
files"));
-  }
-  else {
-    /* It's possible in future other formats will support discard, but
-     * currently (qemu 1.7) none of them do.
-     */
-    NOT_SUPPORTED (g, false,
-                   _("discard cannot be enabled on this drive: "
-                     "qemu does not support discard for '%s'
format files"),
-                   drv->src.format);
-  }
-
-  switch (drv->src.protocol) {
-    /* Protocols which support discard. */
-  case drive_protocol_file:
-  case drive_protocol_gluster:
-  case drive_protocol_iscsi:
-  case drive_protocol_nbd:
-  case drive_protocol_rbd:
-  case drive_protocol_sheepdog: /* XXX depends on server version */
-    break;
-
-    /* Protocols which don't support discard. */
-  case drive_protocol_ftp:
-  case drive_protocol_ftps:
-  case drive_protocol_http:
-  case drive_protocol_https:
-  case drive_protocol_ssh:
-  case drive_protocol_tftp:
-    NOT_SUPPORTED (g, -1,
-                   _("discard cannot be enabled on this drive: "
-                     "protocol '%s' does not support
discard"),
-                   guestfs_int_drive_protocol_to_string
(drv->src.protocol));
-  }
-
-  return true;
-}
-
-static int
 shutdown_direct (guestfs_h *g, void *datav, int check_for_errors)
 {
   struct backend_direct_data *data = datav;
@@ -1506,10 +972,8 @@ shutdown_direct (guestfs_h *g, void *datav, int
check_for_errors)
     data->guestfsd_sock[0] = '\0';
   }
 
-  free (data->qemu_help);
-  data->qemu_help = NULL;
-  free (data->qemu_devices);
-  data->qemu_devices = NULL;
+  guestfs_int_free_qemu_data (data->qemu_data);
+  data->qemu_data = NULL;
 
   return ret;
 }
@@ -1533,7 +997,11 @@ max_disks_direct (guestfs_h *g, void *datav)
 {
   struct backend_direct_data *data = datav;
 
-  if (qemu_supports_virtio_scsi (g, data))
+  data->qemu_data = guestfs_int_test_qemu (g);
+  if (data->qemu_data == NULL)
+    return -1;
+
+  if (guestfs_int_qemu_supports_virtio_scsi (g, data->qemu_data))
     return 255;
   else
     return 27;                  /* conservative estimate */
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index dba28b4..67cbde2 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -120,6 +120,7 @@ struct backend_libvirt_data {
   bool is_kvm;                  /* false = qemu, true = kvm (from
capabilities)*/
   unsigned long libvirt_version; /* libvirt version */
   unsigned long qemu_version;   /* qemu version (from libvirt) */
+  struct qemu_data *qemu_data;
   struct secret *secrets;       /* list of secrets */
   size_t nr_secrets;
   char *uefi_code;		/* UEFI (firmware) code and variables. */
@@ -327,6 +328,10 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
     data->qemu_version = 0;
   }
 
+  data->qemu_data = guestfs_int_init_qemu_version (g,
data->qemu_version);
+  if (data->qemu_data == NULL)
+    goto cleanup;
+
   debug (g, "get libvirt capabilities");
 
   capabilities_xml = virConnectGetCapabilities (conn);
@@ -638,6 +643,9 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   free (params.initrd);
   free (params.appliance_overlay);
 
+  guestfs_int_free_qemu_data (data->qemu_data);
+  data->qemu_data = NULL;
+
   g->state = CONFIG;
 
   return -1;
@@ -1574,7 +1582,7 @@ construct_libvirt_xml_disk_driver_qemu (guestfs_h *g,
      */
     break;
   case discard_enable:
-    if (!guestfs_int_discard_possible (g, drv, data->qemu_version))
+    if (!guestfs_int_discard_possible (g, drv, data->qemu_data))
       return -1;
     /*FALLTHROUGH*/
   case discard_besteffort:
@@ -2070,6 +2078,9 @@ shutdown_libvirt (guestfs_h *g, void *datav, int
check_for_errors)
   free (data->uefi_vars);
   data->uefi_vars = NULL;
 
+  guestfs_int_free_qemu_data (data->qemu_data);
+  data->qemu_data = NULL;
+
   return ret;
 }
 
diff --git a/src/qemu.c b/src/qemu.c
new file mode 100644
index 0000000..feb8bd6
--- /dev/null
+++ b/src/qemu.c
@@ -0,0 +1,619 @@
+/* libguestfs
+ * Copyright (C) 2009-2016 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
+ */
+
+/**
+ * Functions to handle qemu versions and features.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include <string.h>
+#include <inttypes.h>
+#include <unistd.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <assert.h>
+#include <libintl.h>
+
+#include <libxml/uri.h>
+
+#include <pcre.h>
+
+#include "ignore-value.h"
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+#include "guestfs_protocol.h"
+
+COMPILE_REGEXP (re_major_minor, "(\\d+)\\.(\\d+)", 0)
+
+struct qemu_data {
+  char *qemu_help;            /* Output of qemu -help. */
+  char *qemu_devices;         /* Output of qemu -device ? */
+
+  /* qemu version (0, 0 if unable to parse). */
+  int qemu_version_major, qemu_version_minor;
+
+  int virtio_scsi;        /* See function
+                             guestfs_int_qemu_supports_virtio_scsi */
+};
+
+static void parse_qemu_version (guestfs_h *g, struct qemu_data *data);
+static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
+
+/**
+ * Test qemu binary (or wrapper) runs, and do C<qemu -help> so we know
+ * the version of qemu what options this qemu supports, and
+ * C<qemu -device ?> so we know what devices are available.
+ */
+struct qemu_data *
+guestfs_int_test_qemu (guestfs_h *g)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd1 = guestfs_int_new_command (g);
+  CLEANUP_CMD_CLOSE struct command *cmd2 = guestfs_int_new_command (g);
+  int r;
+  struct qemu_data *data;
+
+  data = safe_calloc (g, 1, sizeof *data);
+
+  guestfs_int_cmd_add_arg (cmd1, g->hv);
+  guestfs_int_cmd_add_arg (cmd1, "-display");
+  guestfs_int_cmd_add_arg (cmd1, "none");
+  guestfs_int_cmd_add_arg (cmd1, "-help");
+  guestfs_int_cmd_set_stdout_callback (cmd1, read_all, &data->qemu_help,
+				       CMD_STDOUT_FLAG_WHOLE_BUFFER);
+  r = guestfs_int_cmd_run (cmd1);
+  if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0)
+    goto error;
+
+  parse_qemu_version (g, data);
+
+  guestfs_int_cmd_add_arg (cmd2, g->hv);
+  guestfs_int_cmd_add_arg (cmd2, "-display");
+  guestfs_int_cmd_add_arg (cmd2, "none");
+  guestfs_int_cmd_add_arg (cmd2, "-machine");
+  guestfs_int_cmd_add_arg (cmd2,
+#ifdef MACHINE_TYPE
+                           MACHINE_TYPE ","
+#endif
+                           "accel=kvm:tcg");
+  guestfs_int_cmd_add_arg (cmd2, "-device");
+  guestfs_int_cmd_add_arg (cmd2, "?");
+  guestfs_int_cmd_clear_capture_errors (cmd2);
+  guestfs_int_cmd_set_stderr_to_stdout (cmd2);
+  guestfs_int_cmd_set_stdout_callback (cmd2, read_all,
&data->qemu_devices,
+				       CMD_STDOUT_FLAG_WHOLE_BUFFER);
+  r = guestfs_int_cmd_run (cmd2);
+  if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0)
+    goto error;
+
+  return data;
+
+ error:
+  free (data);
+
+  if (r == -1)
+    return NULL;
+
+  guestfs_int_external_command_failed (g, r, g->hv, NULL);
+  return NULL;
+}
+
+/* Parse the first line of data->qemu_help (if not NULL) into the
+ * major and minor version of qemu, but don't fail if parsing is not
+ * possible.
+ */
+static void
+parse_qemu_version (guestfs_h *g, struct qemu_data *data)
+{
+  CLEANUP_FREE char *major_s = NULL, *minor_s = NULL;
+  int major_i, minor_i;
+
+  data->qemu_version_major = 0;
+  data->qemu_version_minor = 0;
+
+  if (!data->qemu_help)
+    return;
+
+  if (!match2 (g, data->qemu_help, re_major_minor, &major_s,
&minor_s)) {
+  parse_failed:
+    debug (g, "%s: failed to parse qemu version string from the first line
of the output of '%s -help'.  When reporting this bug please include the
-help output.",
+           __func__, g->hv);
+    return;
+  }
+
+  major_i = guestfs_int_parse_unsigned_int (g, major_s);
+  if (major_i == -1)
+    goto parse_failed;
+
+  minor_i = guestfs_int_parse_unsigned_int (g, minor_s);
+  if (minor_i == -1)
+    goto parse_failed;
+
+  data->qemu_version_major = major_i;
+  data->qemu_version_minor = minor_i;
+
+  debug (g, "qemu version %d.%d", major_i, minor_i);
+}
+
+static void
+read_all (guestfs_h *g, void *retv, const char *buf, size_t len)
+{
+  char **ret = retv;
+
+  *ret = safe_strndup (g, buf, len);
+}
+
+/**
+ * This initializes just the C<qemu_version_major> and
+ * C<qemu_version_minor> fields of the struct from the libvirt qemu
+ * version.  Only used by the libvirt backend.
+ */
+struct qemu_data *
+guestfs_int_init_qemu_version (guestfs_h *g, unsigned long qemu_version)
+{
+  struct qemu_data *data;
+
+  data = safe_calloc (g, 1, sizeof *data);
+  data->qemu_version_major = qemu_version / 1000000UL;
+  data->qemu_version_minor = qemu_version / 1000UL % 1000UL;
+
+  return data;
+}
+
+/**
+ * Test if option is supported by qemu command line (just by grepping
+ * the help text).
+ */
+int
+guestfs_int_qemu_supports (guestfs_h *g, const struct qemu_data *data,
+                           const char *option)
+{
+  return strstr (data->qemu_help, option) != NULL;
+}
+
+/**
+ * Test if device is supported by qemu (currently just greps the
+ * C<qemu -device ?> output).
+ */
+int
+guestfs_int_qemu_supports_device (guestfs_h *g,
+                                  const struct qemu_data *data,
+                                  const char *device_name)
+{
+  return strstr (data->qemu_devices, device_name) != NULL;
+}
+
+static int
+old_or_broken_virtio_scsi (guestfs_h *g, const struct qemu_data *data)
+{
+  /* qemu 1.1 claims to support virtio-scsi but in reality it's broken. */
+  if (data->qemu_version_major == 1 && data->qemu_version_minor
< 2)
+    return 1;
+
+  return 0;
+}
+
+/**
+ * Test if qemu supports virtio-scsi.
+ *
+ * Returns C<1> = use virtio-scsi, or C<0> = use virtio-blk.
+ */
+int
+guestfs_int_qemu_supports_virtio_scsi (guestfs_h *g, struct qemu_data *data)
+{
+  int r;
+
+  /* data->virtio_scsi has these values:
+   *   0 = untested (after handle creation)
+   *   1 = supported
+   *   2 = not supported (use virtio-blk)
+   *   3 = test failed (use virtio-blk)
+   */
+  if (data->virtio_scsi == 0) {
+    if (old_or_broken_virtio_scsi (g, data))
+      data->virtio_scsi = 2;
+    else {
+      r = guestfs_int_qemu_supports_device (g, data, VIRTIO_SCSI);
+      if (r > 0)
+        data->virtio_scsi = 1;
+      else if (r == 0)
+        data->virtio_scsi = 2;
+      else
+        data->virtio_scsi = 3;
+    }
+  }
+
+  return data->virtio_scsi == 1;
+}
+
+/**
+ * Escape a qemu parameter.
+ *
+ * Every C<,> becomes C<,,>.  The caller must free the returned
string.
+ */
+char *
+guestfs_int_qemu_escape_param (guestfs_h *g, const char *param)
+{
+  size_t i, len = strlen (param);
+  char *p, *ret;
+
+  ret = p = safe_malloc (g, len*2 + 1); /* max length of escaped name*/
+  for (i = 0; i < len; ++i) {
+    *p++ = param[i];
+    if (param[i] == ',')
+      *p++ = ',';
+  }
+  *p = '\0';
+
+  return ret;
+}
+
+static char *
+make_uri (guestfs_h *g, const char *scheme, const char *user,
+          const char *password,
+          struct drive_server *server, const char *path)
+{
+  xmlURI uri = { .scheme = (char *) scheme,
+                 .user = (char *) user };
+  CLEANUP_FREE char *query = NULL;
+  CLEANUP_FREE char *pathslash = NULL;
+  CLEANUP_FREE char *userauth = NULL;
+
+  /* Need to add a leading '/' to URI paths since xmlSaveUri
doesn't. */
+  if (path != NULL && path[0] != '/') {
+    pathslash = safe_asprintf (g, "/%s", path);
+    uri.path = pathslash;
+  }
+  else
+    uri.path = (char *) path;
+
+  /* Rebuild user:password. */
+  if (user != NULL && password != NULL) {
+    /* Keep the string in an own variable so it can be freed automatically. */
+    userauth = safe_asprintf (g, "%s:%s", user, password);
+    uri.user = userauth;
+  }
+
+  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.
+ *
+ * Note that the qemu parameter is the bit after C<"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 L<qemu-img(1)>).
+ */
+char *
+guestfs_int_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->secret,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_ftps:
+    return make_uri (g, "ftps", src->username, src->secret,
+                     &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, NULL,
+                       &src->servers[0], src->u.exportname);
+    case drive_transport_tcp:
+      return make_uri (g, "gluster+tcp", NULL, NULL,
+                       &src->servers[0], src->u.exportname);
+    case drive_transport_unix:
+      return make_uri (g, "gluster+unix", NULL, NULL,
+                       &src->servers[0], NULL);
+    }
+
+  case drive_protocol_http:
+    return make_uri (g, "http", src->username, src->secret,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_https:
+    return make_uri (g, "https", src->username, src->secret,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_iscsi: {
+    CLEANUP_FREE char *escaped_hostname = NULL;
+    CLEANUP_FREE char *escaped_target = NULL;
+    CLEANUP_FREE char *userauth = NULL;
+    char port_str[16];
+    char *ret;
+
+    escaped_hostname +      (char *) xmlURIEscapeStr(BAD_CAST
src->servers[0].u.hostname,
+                               BAD_CAST "");
+    /* The target string must keep slash as it is, as exportname contains
+     * "iqn/lun".
+     */
+    escaped_target +      (char *) xmlURIEscapeStr(BAD_CAST
src->u.exportname, BAD_CAST "/");
+    if (src->username != NULL && src->secret != NULL)
+      userauth = safe_asprintf (g, "%s%%%s@", src->username,
src->secret);
+    if (src->servers[0].port != 0)
+      snprintf (port_str, sizeof port_str, ":%d",
src->servers[0].port);
+
+    ret = safe_asprintf (g, "iscsi://%s%s%s/%s",
+                         userauth != NULL ? userauth : "",
+                         escaped_hostname,
+                         src->servers[0].port != 0 ? port_str :
"",
+                         escaped_target);
+
+    return ret;
+  }
+
+  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
+      ret = safe_asprintf (g, "%s:exportname=%s", p,
src->u.exportname);
+
+    return ret;
+  }
+
+  case drive_protocol_rbd: {
+    CLEANUP_FREE char *mon_host = NULL, *username = NULL, *secret = NULL;
+    const char *auth;
+    size_t n = 0;
+    size_t i, j;
+
+    /* build the list of all the mon hosts */
+    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";
+
+    return safe_asprintf (g, "rbd:%s%s%s%s%s%s",
+                          src->u.exportname,
+                          src->nr_servers > 0 ? ":mon_host=" :
"",
+                          src->nr_servers > 0 ? mon_host : "",
+                          username ? username : "",
+                          auth,
+                          secret ? secret : "");
+  }
+
+  case drive_protocol_sheepdog:
+    if (src->nr_servers == 0)
+      return safe_asprintf (g, "sheepdog:%s", src->u.exportname);
+    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);
+
+  case drive_protocol_ssh:
+    return make_uri (g, "ssh", src->username, src->secret,
+                     &src->servers[0], src->u.exportname);
+
+  case drive_protocol_tftp:
+    return make_uri (g, "tftp", src->username, src->secret,
+                     &src->servers[0], src->u.exportname);
+  }
+
+  abort ();
+}
+
+/**
+ * Test if discard is both supported by qemu AND possible with the
+ * underlying file or device.  This returns C<1> if discard is
+ * possible.  It returns C<0> if not possible and sets the error to
+ * the reason why.
+ *
+ * This function is called when the user set C<discard ==
"enable">.
+ */
+bool
+guestfs_int_discard_possible (guestfs_h *g, struct drive *drv,
+			      const struct qemu_data *data)
+{
+  /* qemu >= 1.5.  This was the first version that supported the
+   * discard option on -drive at all.
+   */
+  bool qemu15 = guestfs_int_qemu_version_ge (data, 1, 5);
+  /* qemu >= 1.6.  This was the first version that supported unmap on
+   * qcow2 backing files.
+   */
+  bool qemu16 = guestfs_int_qemu_version_ge (data, 1, 6);
+
+  if (!qemu15)
+    NOT_SUPPORTED (g, false,
+                   _("discard cannot be enabled on this drive: "
+                     "qemu < 1.5"));
+
+  /* If it's an overlay, discard is not possible (on the underlying
+   * file).  This has probably been caught earlier since we already
+   * checked that the drive is !readonly.  Nevertheless ...
+   */
+  if (drv->overlay)
+    NOT_SUPPORTED (g, false,
+                   _("discard cannot be enabled on this drive: "
+                     "the drive has a read-only overlay"));
+
+  /* Look at the source format. */
+  if (drv->src.format == NULL) {
+    /* We could autodetect the format, but we don't ... yet. XXX */
+    NOT_SUPPORTED (g, false,
+                   _("discard cannot be enabled on this drive: "
+                     "you have to specify the format of the file"));
+  }
+  else if (STREQ (drv->src.format, "raw"))
+    /* OK */ ;
+  else if (STREQ (drv->src.format, "qcow2")) {
+    if (!qemu16)
+      NOT_SUPPORTED (g, false,
+                     _("discard cannot be enabled on this drive: "
+                       "qemu < 1.6 cannot do discard on qcow2
files"));
+  }
+  else {
+    /* It's possible in future other formats will support discard, but
+     * currently (qemu 1.7) none of them do.
+     */
+    NOT_SUPPORTED (g, false,
+                   _("discard cannot be enabled on this drive: "
+                     "qemu does not support discard for '%s'
format files"),
+                   drv->src.format);
+  }
+
+  switch (drv->src.protocol) {
+    /* Protocols which support discard. */
+  case drive_protocol_file:
+  case drive_protocol_gluster:
+  case drive_protocol_iscsi:
+  case drive_protocol_nbd:
+  case drive_protocol_rbd:
+  case drive_protocol_sheepdog: /* XXX depends on server version */
+    break;
+
+    /* Protocols which don't support discard. */
+  case drive_protocol_ftp:
+  case drive_protocol_ftps:
+  case drive_protocol_http:
+  case drive_protocol_https:
+  case drive_protocol_ssh:
+  case drive_protocol_tftp:
+    NOT_SUPPORTED (g, -1,
+                   _("discard cannot be enabled on this drive: "
+                     "protocol '%s' does not support
discard"),
+                   guestfs_int_drive_protocol_to_string
(drv->src.protocol));
+  }
+
+  return true;
+}
+
+/**
+ * Test if qemu version E<le> some maximum version.
+ */
+bool
+guestfs_int_qemu_version_le (const struct qemu_data *data,
+                             int max_major, int max_minor)
+{
+  return
+    data->qemu_version_major < max_major ||
+    (data->qemu_version_major == max_major &&
+     data->qemu_version_minor <= max_minor);
+}
+
+/**
+ * Test if qemu version E<ge> some minimum version.
+ */
+bool
+guestfs_int_qemu_version_ge (const struct qemu_data *data,
+                             int min_major, int min_minor)
+{
+  return
+    data->qemu_version_major > min_major ||
+    (data->qemu_version_major == min_major &&
+     data->qemu_version_minor >= min_minor);
+}
+
+/**
+ * Free the C<struct qemu_data>.
+ */
+void
+guestfs_int_free_qemu_data (struct qemu_data *data)
+{
+  if (data) {
+    free (data->qemu_help);
+    free (data->qemu_devices);
+    free (data);
+  }
+}
-- 
2.7.4

---
 src/tmpdirs.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/src/tmpdirs.c b/src/tmpdirs.c
index afa3dd4..293e4ea 100644
--- a/src/tmpdirs.c
+++ b/src/tmpdirs.c
@@ -16,6 +16,10 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+/**
+ * Handle temporary directories.
+ */
+
 #include <config.h>
 
 #include <stdio.h>
@@ -31,9 +35,10 @@
 #include "guestfs-internal.h"
 #include "guestfs-internal-actions.h"
 
-/* We need to make all tmpdir paths absolute because lots of places in
+/**
+ * We need to make all tmpdir paths absolute because lots of places in
  * the code assume this.  Do it at the time we set the path or read
- * the environment variable (RHBZ#882417).
+ * the environment variable (L<https://bugzilla.redhat.com/882417>).
  */
 static int
 set_abs_path (guestfs_h *g, const char *tmpdir, char **tmpdir_ret)
@@ -88,7 +93,12 @@ guestfs_impl_set_tmpdir (guestfs_h *g, const char *tmpdir)
   return set_abs_path (g, tmpdir, &g->int_tmpdir);
 }
 
-/* Note this actually calculates the tmpdir, so it never returns NULL. */
+/**
+ * Implements the C<guestfs_get_tmpdir> API.
+ *
+ * Note this actually calculates the tmpdir, so it never returns
+ * C<NULL>.
+ */
 char *
 guestfs_impl_get_tmpdir (guestfs_h *g)
 {
@@ -110,7 +120,11 @@ guestfs_impl_set_cachedir (guestfs_h *g, const char
*cachedir)
   return set_abs_path (g, cachedir, &g->int_cachedir);
 }
 
-/* Note this actually calculates the cachedir, so it never returns NULL. */
+/**
+ * Implements the C<guestfs_get_cachedir> API.
+ *
+ * Note this actually calculates the cachedir, so it never returns
C<NULL>.
+ */
 char *
 guestfs_impl_get_cachedir (guestfs_h *g)
 {
@@ -126,7 +140,12 @@ guestfs_impl_get_cachedir (guestfs_h *g)
   return safe_strdup (g, str);
 }
 
-/* Note this actually calculates the sockdir, so it never returns NULL. */
+/**
+ * Implements the C<guestfs_get_sockdir> API.
+ *
+ * Note this actually calculates the sockdir, so it never returns
+ * C<NULL>.
+ */
 char *
 guestfs_impl_get_sockdir (guestfs_h *g)
 {
@@ -176,9 +195,10 @@ lazy_make_tmpdir (guestfs_h *g, char *(*getdir) (guestfs_h
*g), char **dest)
   return 0;
 }
 
-/* The g->tmpdir (per-handle temporary directory) is not created when
- * the handle is created.  Instead we create it lazily before the
- * first time it is used, or during launch.
+/**
+ * The C<g-E<gt>tmpdir> (per-handle temporary directory) is not
+ * created when the handle is created.  Instead we create it lazily
+ * before the first time it is used, or during launch.
  */
 int
 guestfs_int_lazy_make_tmpdir (guestfs_h *g)
@@ -192,10 +212,13 @@ guestfs_int_lazy_make_sockdir (guestfs_h *g)
   return lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir);
 }
 
-/* Recursively remove a temporary directory.  If removal fails, just
+/**
+ * Recursively remove a temporary directory.  If removal fails, just
  * return (it's a temporary directory so it'll eventually be cleaned
- * up by a temp cleaner).  This is done using "rm -rf" because
that's
- * simpler and safer.
+ * up by a temp cleaner).
+ *
+ * This is implemented using C<rm -rf> because that's simpler and
+ * safer.
  */
 void
 guestfs_int_recursive_remove_dir (guestfs_h *g, const char *dir)
-- 
2.7.4

This is largely code motion.
---
 src/appliance.c        | 40 +++++++-----------------------------
 src/guestfs-internal.h |  1 +
 src/tmpdirs.c          | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/src/appliance.c b/src/appliance.c
index 2cf6374..d293c2b 100644
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -48,7 +48,7 @@ static int dir_contains_files (guestfs_h *g, const char *dir,
...);
 static int contains_old_style_appliance (guestfs_h *g, const char *path, void
*data);
 static int contains_fixed_appliance (guestfs_h *g, const char *path, void
*data);
 static int contains_supermin_appliance (guestfs_h *g, const char *path, void
*data);
-static int build_supermin_appliance (guestfs_h *g, const char *supermin_path,
uid_t uid, char **kernel, char **initrd, char **appliance);
+static int build_supermin_appliance (guestfs_h *g, const char *supermin_path,
char **kernel, char **initrd, char **appliance);
 static int run_supermin_build (guestfs_h *g, const char *lockfile, const char
*appliancedir, const char *supermin_path);
 
 /**
@@ -133,7 +133,6 @@ build_appliance (guestfs_h *g,
                  char **appliance)
 {
   int r;
-  uid_t uid = geteuid ();
   CLEANUP_FREE char *supermin_path = NULL;
   CLEANUP_FREE char *path = NULL;
 
@@ -144,7 +143,7 @@ build_appliance (guestfs_h *g,
 
   if (r == 1)
     /* Step (2): build supermin appliance. */
-    return build_supermin_appliance (g, supermin_path, uid,
+    return build_supermin_appliance (g, supermin_path,
                                      kernel, initrd, appliance);
 
   /* Step (3). */
@@ -212,43 +211,18 @@ contains_supermin_appliance (guestfs_h *g, const char
*path, void *data)
 static int
 build_supermin_appliance (guestfs_h *g,
                           const char *supermin_path,
-                          uid_t uid,
                           char **kernel, char **initrd,
                           char **appliance)
 {
-  CLEANUP_FREE char *tmpdir = guestfs_get_cachedir (g);
   CLEANUP_FREE char *cachedir = NULL, *lockfile = NULL, *appliancedir = NULL;
-  struct stat statbuf;
 
-  cachedir = safe_asprintf (g, "%s/.guestfs-%ju", tmpdir, (uintmax_t)
uid);
-  lockfile = safe_asprintf (g, "%s/lock", cachedir);
+  cachedir = guestfs_int_lazy_make_supermin_appliance_dir (g);
+  if (cachedir == NULL)
+    return -1;
+
   appliancedir = safe_asprintf (g, "%s/appliance.d", cachedir);
+  lockfile = safe_asprintf (g, "%s/lock", cachedir);
 
-  ignore_value (mkdir (cachedir, 0755));
-  ignore_value (chmod (cachedir, 0755)); /* RHBZ#921292 */
-
-  /* See if the cache directory exists and passes some simple checks
-   * to make sure it has not been tampered with.
-   */
-  if (lstat (cachedir, &statbuf) == -1)
-    return 0;
-  if (statbuf.st_uid != uid) {
-    error (g, _("security: cached appliance %s is not owned by UID
%ju"),
-           cachedir, (uintmax_t) uid);
-    return -1;
-  }
-  if (!S_ISDIR (statbuf.st_mode)) {
-    error (g, _("security: cached appliance %s is not a directory (mode
%o)"),
-           cachedir, statbuf.st_mode);
-    return -1;
-  }
-  if ((statbuf.st_mode & 0022) != 0) {
-    error (g, _("security: cached appliance %s is writable by group or
other (mode %o)"),
-           cachedir, statbuf.st_mode);
-    return -1;
-  }
-
-  (void) utimes (cachedir, NULL);
   debug (g, "begin building supermin appliance");
 
   /* Build the appliance if it needs to be built. */
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index dbd0a98..d325f50 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -758,6 +758,7 @@ extern int guestfs_int_set_env_tmpdir (guestfs_h *g, const
char *tmpdir);
 extern int guestfs_int_set_env_runtimedir (guestfs_h *g, const char
*runtimedir);
 extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g);
 extern int guestfs_int_lazy_make_sockdir (guestfs_h *g);
+extern char *guestfs_int_lazy_make_supermin_appliance_dir (guestfs_h *g);
 extern void guestfs_int_remove_tmpdir (guestfs_h *g);
 extern void guestfs_int_remove_sockdir (guestfs_h *g);
 extern void guestfs_int_recursive_remove_dir (guestfs_h *g, const char *dir);
diff --git a/src/tmpdirs.c b/src/tmpdirs.c
index 293e4ea..6c8fe0d 100644
--- a/src/tmpdirs.c
+++ b/src/tmpdirs.c
@@ -213,6 +213,61 @@ guestfs_int_lazy_make_sockdir (guestfs_h *g)
 }
 
 /**
+ * Create the supermin appliance directory under cachedir, if it does
+ * not exist.
+ *
+ * Sanity-check that the permissions on the cachedir are safe, in case
+ * it has been pre-created maliciously or tampered with.
+ *
+ * Returns the directory name which the caller must free.
+ */
+char *
+guestfs_int_lazy_make_supermin_appliance_dir (guestfs_h *g)
+{
+  CLEANUP_FREE char *tmpdir = guestfs_get_cachedir (g);
+  char *ret = NULL;
+  struct stat statbuf;
+  uid_t uid = geteuid ();
+
+  ret = safe_asprintf (g, "%s/.guestfs-%ju", tmpdir, (uintmax_t)
uid);
+
+  ignore_value (mkdir (ret, 0755));
+  ignore_value (chmod (ret, 0755)); /* RHBZ#921292 */
+
+  /* See if the cache directory exists and passes some simple checks
+   * to make sure it has not been tampered with.
+   */
+  if (lstat (ret, &statbuf) == -1) {
+    perrorf (g, _("stat: %s"), ret);
+    free (ret);
+    return NULL;
+  }
+  if (statbuf.st_uid != uid) {
+    error (g, _("security: cached appliance %s is not owned by UID
%ju"),
+           ret, (uintmax_t) uid);
+    free (ret);
+    return NULL;
+  }
+  if (!S_ISDIR (statbuf.st_mode)) {
+    error (g, _("security: cached appliance %s is not a directory (mode
%o)"),
+           ret, statbuf.st_mode);
+    free (ret);
+    return NULL;
+  }
+  if ((statbuf.st_mode & 0022) != 0) {
+    error (g, _("security: cached appliance %s is writable by group or
other (mode %o)"),
+           ret, statbuf.st_mode);
+    free (ret);
+    return NULL;
+  }
+
+  /* "Touch" the directory. */
+  ignore_value (utimes (ret, NULL));
+
+  return ret;
+}
+
+/**
  * Recursively remove a temporary directory.  If removal fails, just
  * return (it's a temporary directory so it'll eventually be cleaned
  * up by a temp cleaner).
-- 
2.7.4

qemu feature detection takes about 95ms on my laptop.  The overhead is
almost all due to the time taken by the glibc link loader opening the
170+ libraries that qemu is linked to (×2 because we need to run qemu
twice).

Fixing that is seriously hard work.

Therefore memoize the results of guestfs_int_test_qemu.

This is keyed on the size and mtime of the qemu binary, so if the user
changes the qemu binary (eg. setting LIBGUESTFS_HV) we discard the
memoized result and rerun the qemu commands.  There is also a
generation number so we can bump the generation in future versions of
libguestfs to invalidate all previously cached data.

The memo is stored in the supermin cache directory (eg. /var/tmp/.guestfs-*)
in the files:

  qemu.stat     Result of 'stat(2)' of the qemu binary
  qemu.help     qemu -help output
  qemu.devices  qemu -devices ? output

Note these files are only stored when using the 'direct' backend.  For
the libvirt backend, libvirt itself memoizes this data in its own
place.
---
 src/qemu.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 146 insertions(+), 8 deletions(-)

diff --git a/src/qemu.c b/src/qemu.c
index feb8bd6..259a6fd 100644
--- a/src/qemu.c
+++ b/src/qemu.c
@@ -58,23 +58,163 @@ struct qemu_data {
                              guestfs_int_qemu_supports_virtio_scsi */
 };
 
+static int test_qemu (guestfs_h *g, struct qemu_data *data);
 static void parse_qemu_version (guestfs_h *g, struct qemu_data *data);
 static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
 
+/* This is saved in the qemu.stat file, so if we decide to change the
+ * test_qemu memoization format/data in future, we should increment
+ * this to discard any memoized data cached by previous versions of
+ * libguestfs.
+ */
+#define MEMO_GENERATION 1
+
 /**
  * Test qemu binary (or wrapper) runs, and do C<qemu -help> so we know
  * the version of qemu what options this qemu supports, and
  * C<qemu -device ?> so we know what devices are available.
+ *
+ * This caches the results in the cachedir so that as long as the qemu
+ * binary does not change, calling this is effectively free.
  */
 struct qemu_data *
 guestfs_int_test_qemu (guestfs_h *g)
 {
+  struct qemu_data *data;
+  struct stat statbuf;
+  CLEANUP_FREE char *cachedir = NULL, *qemu_stat_filename = NULL,
+    *qemu_help_filename = NULL, *qemu_devices_filename = NULL;
+  FILE *fp;
+  int generation;
+  uint64_t prev_size, prev_mtime;
+
+  if (stat (g->hv, &statbuf) == -1) {
+    perrorf (g, "stat: %s", g->hv);
+    return NULL;
+  }
+
+  cachedir = guestfs_int_lazy_make_supermin_appliance_dir (g);
+  if (cachedir == NULL)
+    return NULL;
+
+  qemu_stat_filename = safe_asprintf (g, "%s/qemu.stat", cachedir);
+  qemu_help_filename = safe_asprintf (g, "%s/qemu.help", cachedir);
+  qemu_devices_filename = safe_asprintf (g, "%s/qemu.devices",
cachedir);
+
+  /* Did we previously test the same version of qemu? */
+  debug (g, "checking for previously cached test results of %s, in
%s",
+         g->hv, cachedir);
+
+  fp = fopen (qemu_stat_filename, "r");
+  if (fp == NULL)
+    goto do_test;
+  if (fscanf (fp, "%d %" SCNu64 " %" SCNu64,
+              &generation, &prev_size, &prev_mtime) != 3) {
+    fclose (fp);
+    goto do_test;
+  }
+  fclose (fp);
+
+  if (generation == MEMO_GENERATION &&
+      (uint64_t) statbuf.st_size == prev_size &&
+      (uint64_t) statbuf.st_mtime == prev_mtime) {
+    /* Same binary as before, so read the previously cached qemu -help
+     * and qemu -devices ? output.
+     */
+    if (access (qemu_help_filename, R_OK) == -1 ||
+        access (qemu_devices_filename, R_OK) == -1)
+      goto do_test;
+
+    debug (g, "loading previously cached test results");
+
+    data = safe_calloc (g, 1, sizeof *data);
+
+    if (guestfs_int_read_whole_file (g, qemu_help_filename,
+                                     &data->qemu_help, NULL) == -1) {
+      guestfs_int_free_qemu_data (data);
+      return NULL;
+    }
+
+    parse_qemu_version (g, data);
+
+    if (guestfs_int_read_whole_file (g, qemu_devices_filename,
+                                     &data->qemu_devices, NULL) == -1) {
+      guestfs_int_free_qemu_data (data);
+      return NULL;
+    }
+
+    return data;
+  }
+
+ do_test:
+  data = safe_calloc (g, 1, sizeof *data);
+
+  if (test_qemu (g, data) == -1) {
+    free (data);
+    return NULL;
+  }
+
+  /* Now memoize the qemu output in the cache directory. */
+  debug (g, "saving test results");
+
+  fp = fopen (qemu_help_filename, "w");
+  if (fp == NULL) {
+  help_error:
+    perrorf (g, "%s", qemu_help_filename);
+    if (fp != NULL) fclose (fp);
+    guestfs_int_free_qemu_data (data);
+    return NULL;
+  }
+  if (fprintf (fp, "%s", data->qemu_help) == -1)
+    goto help_error;
+  if (fclose (fp) == -1)
+    goto help_error;
+
+  fp = fopen (qemu_devices_filename, "w");
+  if (fp == NULL) {
+  devices_error:
+    perrorf (g, "%s", qemu_devices_filename);
+    if (fp != NULL) fclose (fp);
+    guestfs_int_free_qemu_data (data);
+    return NULL;
+  }
+  if (fprintf (fp, "%s", data->qemu_devices) == -1)
+    goto devices_error;
+  if (fclose (fp) == -1)
+    goto devices_error;
+
+  /* Write the qemu.stat file last so that its presence indicates that
+   * the qemu.help and qemu.devices files ought to exist.
+   */
+  fp = fopen (qemu_stat_filename, "w");
+  if (fp == NULL) {
+  stat_error:
+    perrorf (g, "%s", qemu_stat_filename);
+    if (fp != NULL) fclose (fp);
+    guestfs_int_free_qemu_data (data);
+    return NULL;
+  }
+  /* The path to qemu is stored for information only, it is not
+   * used when we parse the file.
+   */
+  if (fprintf (fp, "%d %" PRIu64 " %" PRIu64 "
%s\n",
+               MEMO_GENERATION,
+               (uint64_t) statbuf.st_size,
+               (uint64_t) statbuf.st_mtime,
+               g->hv) == -1)
+    goto stat_error;
+  if (fclose (fp) == -1)
+    goto stat_error;
+
+  return data;
+}
+
+static int
+test_qemu (guestfs_h *g, struct qemu_data *data)
+{
   CLEANUP_CMD_CLOSE struct command *cmd1 = guestfs_int_new_command (g);
   CLEANUP_CMD_CLOSE struct command *cmd2 = guestfs_int_new_command (g);
   int r;
-  struct qemu_data *data;
-
-  data = safe_calloc (g, 1, sizeof *data);
 
   guestfs_int_cmd_add_arg (cmd1, g->hv);
   guestfs_int_cmd_add_arg (cmd1, "-display");
@@ -107,16 +247,14 @@ guestfs_int_test_qemu (guestfs_h *g)
   if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0)
     goto error;
 
-  return data;
+  return 0;
 
  error:
-  free (data);
-
   if (r == -1)
-    return NULL;
+    return -1;
 
   guestfs_int_external_command_failed (g, r, g->hv, NULL);
-  return NULL;
+  return -1;
 }
 
 /* Parse the first line of data->qemu_help (if not NULL) into the
-- 
2.7.4

On Thursday 12 May 2016 22:26:23 Richard W.M. Jones
wrote:
> ---
>  src/tmpdirs.c | 45 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 11 deletions(-)
LGTM.

Thanks,
-- 
Pino Toscano

On Thursday 12 May 2016 22:26:24 Richard W.M. Jones
wrote:
> This is largely code motion.
> ---
>  src/appliance.c        | 40 +++++++-----------------------------
>  src/guestfs-internal.h |  1 +
>  src/tmpdirs.c          | 55
++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 33 deletions(-)
LGTM.

Thanks,
-- 
Pino Toscano

On Thursday 12 May 2016 22:26:25 Richard W.M. Jones
wrote:
> qemu feature detection takes about 95ms on my laptop.  The overhead is
> almost all due to the time taken by the glibc link loader opening the
> 170+ libraries that qemu is linked to (×2 because we need to run qemu
> twice).
> 
> Fixing that is seriously hard work.
> 
> Therefore memoize the results of guestfs_int_test_qemu.
> 
> This is keyed on the size and mtime of the qemu binary, so if the user
> changes the qemu binary (eg. setting LIBGUESTFS_HV) we discard the
> memoized result and rerun the qemu commands.  There is also a
> generation number so we can bump the generation in future versions of
> libguestfs to invalidate all previously cached data.
> 
> The memo is stored in the supermin cache directory (eg.
/var/tmp/.guestfs-*)
> in the files:
> 
>   qemu.stat     Result of 'stat(2)' of the qemu binary
>   qemu.help     qemu -help output
>   qemu.devices  qemu -devices ? output
> 
> Note these files are only stored when using the 'direct' backend. 
For
> the libvirt backend, libvirt itself memoizes this data in its own
> place.
> ---
The idea looks ok, just one note below.
>  src/qemu.c | 154
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 146 insertions(+), 8 deletions(-)
> 
> diff --git a/src/qemu.c b/src/qemu.c
> index feb8bd6..259a6fd 100644
> --- a/src/qemu.c
> +++ b/src/qemu.c
> @@ -58,23 +58,163 @@ struct qemu_data {
>                               guestfs_int_qemu_supports_virtio_scsi */
>  };
>  
> +static int test_qemu (guestfs_h *g, struct qemu_data *data);
>  static void parse_qemu_version (guestfs_h *g, struct qemu_data *data);
>  static void read_all (guestfs_h *g, void *retv, const char *buf, size_t
len);
>  
> +/* This is saved in the qemu.stat file, so if we decide to change the
> + * test_qemu memoization format/data in future, we should increment
> + * this to discard any memoized data cached by previous versions of
> + * libguestfs.
> + */
> +#define MEMO_GENERATION 1
> +
>  /**
>   * Test qemu binary (or wrapper) runs, and do C<qemu -help> so we
know
>   * the version of qemu what options this qemu supports, and
>   * C<qemu -device ?> so we know what devices are available.
> + *
> + * This caches the results in the cachedir so that as long as the qemu
> + * binary does not change, calling this is effectively free.
>   */
>  struct qemu_data *
>  guestfs_int_test_qemu (guestfs_h *g)
>  {
> +  struct qemu_data *data;
> +  struct stat statbuf;
> +  CLEANUP_FREE char *cachedir = NULL, *qemu_stat_filename = NULL,
> +    *qemu_help_filename = NULL, *qemu_devices_filename = NULL;
> +  FILE *fp;
> +  int generation;
> +  uint64_t prev_size, prev_mtime;
> +
> +  if (stat (g->hv, &statbuf) == -1) {
> +    perrorf (g, "stat: %s", g->hv);
> +    return NULL;
> +  }
> +
> +  cachedir = guestfs_int_lazy_make_supermin_appliance_dir (g);
> +  if (cachedir == NULL)
> +    return NULL;
> +
> +  qemu_stat_filename = safe_asprintf (g, "%s/qemu.stat",
cachedir);
> +  qemu_help_filename = safe_asprintf (g, "%s/qemu.help",
cachedir);
> +  qemu_devices_filename = safe_asprintf (g, "%s/qemu.devices",
cachedir);
Instead of writing the generation number to the stat file, what about
doing a simplier approach, i.e. add it to the filenames:
  - $cachedir/qemu.$generation.stat
  - $cachedir/qemu.$generation.help
  - etc

Less data to read, fopen will just open the data for the right
generation.

-- 
Pino Toscano