Libguestfs - Sep 2017 - [PATCH v2 2/5] lib: qemu: Factor out common code for reading and writing cache files.

Patches 1-4 are almost the same as they are when previously posted
here:

https://www.redhat.com/archives/libguestfs/2017-September/msg00039.html

Patch 5 actually uses the mandatory locking test to turn off locking
in the narrow case where a drive is opened readonly, and then only for
the drive being inspected.

Passes ordinary tests (‘check-direct’ and ‘check-valgrind-direct’).

Rich.

Rather unnecessarily this function returned the parsed qemu version.
This complicates further refactoring, so I have changed the function
not to return this, and instead there is a separate function you have
to call to get the version struct (‘guestfs_int_qemu_version’).

Apart from a tiny amount of extra copying this is simply refactoring
of the interface between the direct-mode backend and the qemu query
functions.
---
 lib/guestfs-internal.h |  3 ++-
 lib/launch-direct.c    |  5 ++++-
 lib/qemu.c             | 31 +++++++++++++++++++------------
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 7269fbeba..2ca258cb3 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -979,7 +979,8 @@ 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, struct version
*qemu_version);
+extern struct qemu_data *guestfs_int_test_qemu (guestfs_h *g);
+extern struct version guestfs_int_qemu_version (guestfs_h *g, struct qemu_data
*);
 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 char *guestfs_int_drive_source_qemu_param (guestfs_h *g, const struct
drive_source *src);
diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index 3b848165c..9f85013f9 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -402,9 +402,12 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
 
   /* Get qemu help text and version. */
   if (data->qemu_data == NULL) {
-    data->qemu_data = guestfs_int_test_qemu (g, &data->qemu_version);
+    data->qemu_data = guestfs_int_test_qemu (g);
     if (data->qemu_data == NULL)
       goto cleanup0;
+    data->qemu_version = guestfs_int_qemu_version (g, data->qemu_data);
+    debug (g, "qemu version: %d.%d",
+           data->qemu_version.v_major, data->qemu_version.v_minor);
   }
 
   /* Using virtio-serial, we need to create a local Unix domain socket
diff --git a/lib/qemu.c b/lib/qemu.c
index 41098a20b..48e283d48 100644
--- a/lib/qemu.c
+++ b/lib/qemu.c
@@ -46,9 +46,12 @@
 struct qemu_data {
   char *qemu_help;              /* Output of qemu -help. */
   char *qemu_devices;           /* Output of qemu -device ? */
+
+  /* The following fields are derived from the fields above. */
+  struct version qemu_version;  /* Parsed qemu version number. */
 };
 
-static int test_qemu (guestfs_h *g, struct qemu_data *data, struct version
*qemu_version);
+static int test_qemu (guestfs_h *g, struct qemu_data *data);
 static void parse_qemu_version (guestfs_h *g, const char *, struct version
*qemu_version);
 static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
 
@@ -64,14 +67,11 @@ static void read_all (guestfs_h *g, void *retv, const char
*buf, size_t len);
  * the version of qemu what options this qemu supports, and
  * C<qemu -device ?> so we know what devices are available.
  *
- * The version number of qemu (from the C<-help> output) is saved in
- * C<&qemu_version>.
- *
  * 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 version *qemu_version)
+guestfs_int_test_qemu (guestfs_h *g)
 {
   struct qemu_data *data;
   struct stat statbuf;
@@ -128,7 +128,7 @@ guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version)
       return NULL;
     }
 
-    parse_qemu_version (g, data->qemu_help, qemu_version);
+    parse_qemu_version (g, data->qemu_help, &data->qemu_version);
 
     if (guestfs_int_read_whole_file (g, qemu_devices_filename,
                                      &data->qemu_devices, NULL) == -1) {
@@ -142,11 +142,13 @@ guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version)
  do_test:
   data = safe_calloc (g, 1, sizeof *data);
 
-  if (test_qemu (g, data, qemu_version) == -1) {
+  if (test_qemu (g, data) == -1) {
     guestfs_int_free_qemu_data (data);
     return NULL;
   }
 
+  parse_qemu_version (g, data->qemu_help, &data->qemu_version);
+
   /* Now memoize the qemu output in the cache directory. */
   debug (g, "saving test results");
 
@@ -203,7 +205,7 @@ guestfs_int_test_qemu (guestfs_h *g, struct version
*qemu_version)
 }
 
 static int
-test_qemu (guestfs_h *g, struct qemu_data *data, struct version *qemu_version)
+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);
@@ -219,8 +221,6 @@ test_qemu (guestfs_h *g, struct qemu_data *data, struct
version *qemu_version)
   if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0)
     goto error;
 
-  parse_qemu_version (g, data->qemu_help, qemu_version);
-
   guestfs_int_cmd_add_arg (cmd2, g->hv);
   guestfs_int_cmd_add_arg (cmd2, "-display");
   guestfs_int_cmd_add_arg (cmd2, "none");
@@ -265,8 +265,6 @@ parse_qemu_version (guestfs_h *g, const char *qemu_help,
            __func__, g->hv);
     return;
   }
-
-  debug (g, "qemu version %d.%d", qemu_version->v_major,
qemu_version->v_minor);
 }
 
 static void
@@ -278,6 +276,15 @@ read_all (guestfs_h *g, void *retv, const char *buf, size_t
len)
 }
 
 /**
+ * Return the parsed version of qemu.
+ */
+struct version
+guestfs_int_qemu_version (guestfs_h *g, struct qemu_data *data)
+{
+  return data->qemu_version;
+}
+
+/**
  * Test if option is supported by qemu command line (just by grepping
  * the help text).
  */
-- 
2.13.2

The previous code duplicated a lot of common code for reading and
writing the cache file per data field.  This change simply factors out
that common code.  This makes it simpler to add new tests in future.

This is just refactoring, it should have no effect.
---
 lib/qemu.c | 375 +++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 238 insertions(+), 137 deletions(-)

diff --git a/lib/qemu.c b/lib/qemu.c
index 48e283d48..bdd9947a8 100644
--- a/lib/qemu.c
+++ b/lib/qemu.c
@@ -44,6 +44,10 @@
 #include "guestfs_protocol.h"
 
 struct qemu_data {
+  int generation;               /* MEMO_GENERATION read from qemu.stat */
+  uint64_t prev_size;           /* Size of qemu binary when cached. */
+  uint64_t prev_mtime;          /* mtime of qemu binary when cached. */
+
   char *qemu_help;              /* Output of qemu -help. */
   char *qemu_devices;           /* Output of qemu -device ? */
 
@@ -51,9 +55,42 @@ struct qemu_data {
   struct version qemu_version;  /* Parsed qemu version number. */
 };
 
-static int test_qemu (guestfs_h *g, struct qemu_data *data);
+static int test_qemu_help (guestfs_h *g, struct qemu_data *data);
+static int read_cache_qemu_help (guestfs_h *g, struct qemu_data *data, const
char *filename);
+static int write_cache_qemu_help (guestfs_h *g, const struct qemu_data *data,
const char *filename);
+static int test_qemu_devices (guestfs_h *g, struct qemu_data *data);
+static int read_cache_qemu_devices (guestfs_h *g, struct qemu_data *data, const
char *filename);
+static int write_cache_qemu_devices (guestfs_h *g, const struct qemu_data
*data, const char *filename);
+static int read_cache_qemu_stat (guestfs_h *g, struct qemu_data *data, const
char *filename);
+static int write_cache_qemu_stat (guestfs_h *g, const struct qemu_data *data,
const char *filename);
 static void parse_qemu_version (guestfs_h *g, const char *, struct version
*qemu_version);
 static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
+static int generic_read_cache (guestfs_h *g, const char *filename, char
**strp);
+static int generic_write_cache (guestfs_h *g, const char *filename, const char
*str);
+
+/* This structure abstracts the data we are reading from qemu and how
+ * we get it.
+ */
+static const struct qemu_fields {
+  const char *name;
+  /* Function to perform the test on g->hv. */
+  int (*test) (guestfs_h *g, struct qemu_data *data);
+
+  /* Functions to read and write the cache file.
+   * read_cache returns -1 = error, 0 = no cache, 1 = cache data read.
+   * write_cache returns -1 = error, 0 = success.
+   */
+  int (*read_cache) (guestfs_h *g, struct qemu_data *data,
+                     const char *filename);
+  int (*write_cache) (guestfs_h *g, const struct qemu_data *data,
+                      const char *filename);
+} qemu_fields[] = {
+  { "help",
+    test_qemu_help, read_cache_qemu_help, write_cache_qemu_help },
+  { "devices",
+    test_qemu_devices, read_cache_qemu_devices, write_cache_qemu_devices },
+};
+#define NR_FIELDS (sizeof qemu_fields / sizeof qemu_fields[0])
 
 /* 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
@@ -63,9 +100,9 @@ static void read_all (guestfs_h *g, void *retv, const char
*buf, size_t len);
 #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.
+ * Test that the qemu binary (or wrapper) runs, and do C<qemu -help>
+ * and other commands so we can find out the version of qemu and what
+ * options this qemu supports.
  *
  * This caches the results in the cachedir so that as long as the qemu
  * binary does not change, calling this is effectively free.
@@ -73,12 +110,12 @@ static void read_all (guestfs_h *g, void *retv, const char
*buf, size_t len);
 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;
-  int generation;
-  uint64_t prev_size, prev_mtime;
+  struct qemu_data *data;
+  CLEANUP_FREE char *cachedir = NULL;
+  CLEANUP_FREE char *stat_filename = NULL;
+  int r;
+  size_t i;
 
   if (stat (g->hv, &statbuf) == -1) {
     perrorf (g, "stat: %s", g->hv);
@@ -89,165 +126,198 @@ guestfs_int_test_qemu (guestfs_h *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);
 
-  {
-    CLEANUP_FCLOSE FILE *fp = NULL;
-    fp = fopen (qemu_stat_filename, "r");
-    if (fp == NULL)
-      goto do_test;
-    if (fscanf (fp, "%d %" SCNu64 " %" SCNu64,
-                &generation, &prev_size, &prev_mtime) != 3) {
-      goto do_test;
-    }
-  }
-
-  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->qemu_help, &data->qemu_version);
-
-    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) {
-    guestfs_int_free_qemu_data (data);
-    return NULL;
+  stat_filename = safe_asprintf (g, "%s/qemu.stat", cachedir);
+  r = read_cache_qemu_stat (g, data, stat_filename);
+  if (r == -1)
+    goto error;
+  if (r == 0)
+    goto do_test;
+
+  if (data->generation != MEMO_GENERATION ||
+      data->prev_size != (uint64_t) statbuf.st_size ||
+      data->prev_mtime != (uint64_t) statbuf.st_mtime)
+    goto do_test;
+
+  debug (g, "loading previously cached test results");
+
+  for (i = 0; i < NR_FIELDS; ++i) {
+    CLEANUP_FREE char *filename +      safe_asprintf (g,
"%s/qemu.%s", cachedir, qemu_fields[i].name);
+    r = qemu_fields[i].read_cache (g, data, filename);
+    if (r == -1)
+      goto error;
+    if (r == 0) /* cache gone, maybe deleted by the tmp cleaner */
+      goto do_test;
   }
 
-  parse_qemu_version (g, data->qemu_help, &data->qemu_version);
+  goto out;
+
+ do_test:
+  for (i = 0; i < NR_FIELDS; ++i) {
+    if (qemu_fields[i].test (g, data) == -1)
+      goto error;
+  }
 
   /* Now memoize the qemu output in the cache directory. */
   debug (g, "saving test results");
 
-  {
-    CLEANUP_FCLOSE FILE *fp = NULL;
-    fp = fopen (qemu_help_filename, "w");
-    if (fp == NULL) {
-    help_error:
-      perrorf (g, "%s", qemu_help_filename);
-      guestfs_int_free_qemu_data (data);
-      return NULL;
-    }
-    if (fprintf (fp, "%s", data->qemu_help) == -1)
-      goto help_error;
+  for (i = 0; i < NR_FIELDS; ++i) {
+    CLEANUP_FREE char *filename +      safe_asprintf (g,
"%s/qemu.%s", cachedir, qemu_fields[i].name);
+    if (qemu_fields[i].write_cache (g, data, filename) == -1)
+      goto error;
   }
 
-  {
-    CLEANUP_FCLOSE FILE *fp = NULL;
-    fp = fopen (qemu_devices_filename, "w");
-    if (fp == NULL) {
-    devices_error:
-      perrorf (g, "%s", qemu_devices_filename);
-      guestfs_int_free_qemu_data (data);
-      return NULL;
-    }
-    if (fprintf (fp, "%s", data->qemu_devices) == -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.
+   */
+  data->generation = MEMO_GENERATION;
+  data->prev_size = statbuf.st_size;
+  data->prev_mtime = statbuf.st_mtime;
+  if (write_cache_qemu_stat (g, data, stat_filename) == -1)
+    goto error;
 
-  {
-    /* Write the qemu.stat file last so that its presence indicates that
-     * the qemu.help and qemu.devices files ought to exist.
-     */
-    CLEANUP_FCLOSE FILE *fp = NULL;
-    fp = fopen (qemu_stat_filename, "w");
-    if (fp == NULL) {
-    stat_error:
-      perrorf (g, "%s", qemu_stat_filename);
-      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;
-  }
+ out:
+  /* Derived fields. */
+  parse_qemu_version (g, data->qemu_help, &data->qemu_version);
 
   return data;
+
+ error:
+  guestfs_int_free_qemu_data (data);
+  return NULL;
 }
 
 static int
-test_qemu (guestfs_h *g, struct qemu_data *data)
+test_qemu_help (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);
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
   int r;
 
-  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;
+  guestfs_int_cmd_add_arg (cmd, g->hv);
+  guestfs_int_cmd_add_arg (cmd, "-display");
+  guestfs_int_cmd_add_arg (cmd, "none");
+  guestfs_int_cmd_add_arg (cmd, "-help");
+  guestfs_int_cmd_set_stdout_callback (cmd, read_all, &data->qemu_help,
+                                       CMD_STDOUT_FLAG_WHOLE_BUFFER);
+  r = guestfs_int_cmd_run (cmd);
+  if (r == -1)
+    return -1;
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    guestfs_int_external_command_failed (g, r, g->hv, NULL);
+    return -1;
+  }
+  return 0;
+}
+
+static int
+read_cache_qemu_help (guestfs_h *g, struct qemu_data *data,
+                      const char *filename)
+{
+  return generic_read_cache (g, filename, &data->qemu_help);
+}
+
+static int
+write_cache_qemu_help (guestfs_h *g, const struct qemu_data *data,
+                       const char *filename)
+{
+  return generic_write_cache (g, filename, data->qemu_help);
+}
+
+static int
+test_qemu_devices (guestfs_h *g, struct qemu_data *data)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
+  int r;
 
-  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,
+  guestfs_int_cmd_add_arg (cmd, g->hv);
+  guestfs_int_cmd_add_arg (cmd, "-display");
+  guestfs_int_cmd_add_arg (cmd, "none");
+  guestfs_int_cmd_add_arg (cmd, "-machine");
+  guestfs_int_cmd_add_arg (cmd,
 #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:
+  guestfs_int_cmd_add_arg (cmd, "-device");
+  guestfs_int_cmd_add_arg (cmd, "?");
+  guestfs_int_cmd_clear_capture_errors (cmd);
+  guestfs_int_cmd_set_stderr_to_stdout (cmd);
+  guestfs_int_cmd_set_stdout_callback (cmd, read_all,
&data->qemu_devices,
+                                       CMD_STDOUT_FLAG_WHOLE_BUFFER);
+  r = guestfs_int_cmd_run (cmd);
   if (r == -1)
     return -1;
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    guestfs_int_external_command_failed (g, r, g->hv, NULL);
+    return -1;
+  }
+  return 0;
+}
 
-  guestfs_int_external_command_failed (g, r, g->hv, NULL);
-  return -1;
+static int
+read_cache_qemu_devices (guestfs_h *g, struct qemu_data *data,
+                         const char *filename)
+{
+  return generic_read_cache (g, filename, &data->qemu_devices);
+}
+
+static int
+write_cache_qemu_devices (guestfs_h *g, const struct qemu_data *data,
+                          const char *filename)
+{
+  return generic_write_cache (g, filename, data->qemu_devices);
+}
+
+static int
+read_cache_qemu_stat (guestfs_h *g, struct qemu_data *data,
+                      const char *filename)
+{
+  CLEANUP_FCLOSE FILE *fp = fopen (filename, "r");
+  if (fp == NULL) {
+    if (errno == ENOENT)
+      return 0;                 /* no cache, run the test instead */
+    perrorf (g, "%s", filename);
+    return -1;
+  }
+
+  if (fscanf (fp, "%d %" SCNu64 " %" SCNu64,
+              &data->generation,
+              &data->prev_size,
+              &data->prev_mtime) != 3)
+    return 0;
+
+  return 1;
+}
+
+static int
+write_cache_qemu_stat (guestfs_h *g, const struct qemu_data *data,
+                       const char *filename)
+{
+  CLEANUP_FCLOSE FILE *fp = fopen (filename, "w");
+  if (fp == NULL) {
+    perrorf (g, "%s", filename);
+    return -1;
+  }
+  /* 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",
+               data->generation,
+               data->prev_size,
+               data->prev_mtime,
+               g->hv) == -1) {
+    perrorf (g, "%s: write", filename);
+    return -1;
+  }
+
+  return 0;
 }
 
 /**
@@ -267,6 +337,37 @@ parse_qemu_version (guestfs_h *g, const char *qemu_help,
   }
 }
 
+/**
+ * Generic functions for reading and writing the cache files, used
+ * where we are just reading and writing plain text strings.
+ */
+static int
+generic_read_cache (guestfs_h *g, const char *filename, char **strp)
+{
+  if (access (filename, R_OK) == -1 && errno == ENOENT)
+    return 0;                   /* no cache, run the test instead */
+  if (guestfs_int_read_whole_file (g, filename, strp, NULL) == -1)
+    return -1;
+  return 1;
+}
+
+static int
+generic_write_cache (guestfs_h *g, const char *filename, const char *str)
+{
+  CLEANUP_FCLOSE FILE *fp = fopen (filename, "w");
+  if (fp == NULL) {
+    perrorf (g, "%s", filename);
+    return -1;
+  }
+
+  if (fprintf (fp, "%s", str) == -1) {
+    perrorf (g, "%s: write", filename);
+    return -1;
+  }
+
+  return 0;
+}
+
 static void
 read_all (guestfs_h *g, void *retv, const char *buf, size_t len)
 {
-- 
2.13.2

This adds two extra tests using QMP (the QEMU Monitor Protocol).  This
allows us to get extra information about the qemu binary beyond what
is available from the version number or ‘qemu -help’.
---
 lib/qemu.c | 177 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 175 insertions(+), 2 deletions(-)

diff --git a/lib/qemu.c b/lib/qemu.c
index bdd9947a8..34775041f 100644
--- a/lib/qemu.c
+++ b/lib/qemu.c
@@ -37,6 +37,8 @@
 
 #include <libxml/uri.h>
 
+#include <yajl/yajl_tree.h>
+
 #include "ignore-value.h"
 
 #include "guestfs.h"
@@ -50,9 +52,13 @@ struct qemu_data {
 
   char *qemu_help;              /* Output of qemu -help. */
   char *qemu_devices;           /* Output of qemu -device ? */
+  char *qmp_commands;           /* Output of QMP query-commands. */
+  char *qmp_schema;             /* Output of QMP query-qmp-schema. */
 
   /* The following fields are derived from the fields above. */
   struct version qemu_version;  /* Parsed qemu version number. */
+  yajl_val qmp_commands_tree;   /* qmp_commands parsed into a JSON tree */
+  yajl_val qmp_schema_tree;     /* qmp_schema parsed into a JSON tree */
 };
 
 static int test_qemu_help (guestfs_h *g, struct qemu_data *data);
@@ -61,12 +67,20 @@ static int write_cache_qemu_help (guestfs_h *g, const struct
qemu_data *data, co
 static int test_qemu_devices (guestfs_h *g, struct qemu_data *data);
 static int read_cache_qemu_devices (guestfs_h *g, struct qemu_data *data, const
char *filename);
 static int write_cache_qemu_devices (guestfs_h *g, const struct qemu_data
*data, const char *filename);
+static int test_qmp_commands (guestfs_h *g, struct qemu_data *data);
+static int read_cache_qmp_commands (guestfs_h *g, struct qemu_data *data, const
char *filename);
+static int write_cache_qmp_commands (guestfs_h *g, const struct qemu_data
*data, const char *filename);
+static int test_qmp_schema (guestfs_h *g, struct qemu_data *data);
+static int read_cache_qmp_schema (guestfs_h *g, struct qemu_data *data, const
char *filename);
+static int write_cache_qmp_schema (guestfs_h *g, const struct qemu_data *data,
const char *filename);
 static int read_cache_qemu_stat (guestfs_h *g, struct qemu_data *data, const
char *filename);
 static int write_cache_qemu_stat (guestfs_h *g, const struct qemu_data *data,
const char *filename);
 static void parse_qemu_version (guestfs_h *g, const char *, struct version
*qemu_version);
+static void parse_json (guestfs_h *g, const char *, yajl_val *);
 static void read_all (guestfs_h *g, void *retv, const char *buf, size_t len);
 static int generic_read_cache (guestfs_h *g, const char *filename, char
**strp);
 static int generic_write_cache (guestfs_h *g, const char *filename, const char
*str);
+static int generic_qmp_test (guestfs_h *g, struct qemu_data *data, const char
*qmp_command, char **outp);
 
 /* This structure abstracts the data we are reading from qemu and how
  * we get it.
@@ -89,6 +103,10 @@ static const struct qemu_fields {
     test_qemu_help, read_cache_qemu_help, write_cache_qemu_help },
   { "devices",
     test_qemu_devices, read_cache_qemu_devices, write_cache_qemu_devices },
+  { "qmp-commands",
+    test_qmp_commands, read_cache_qmp_commands, write_cache_qmp_commands },
+  { "qmp-schema",
+    test_qmp_schema, read_cache_qmp_schema, write_cache_qmp_schema },
 };
 #define NR_FIELDS (sizeof qemu_fields / sizeof qemu_fields[0])
 
@@ -97,7 +115,7 @@ static const struct qemu_fields {
  * this to discard any memoized data cached by previous versions of
  * libguestfs.
  */
-#define MEMO_GENERATION 1
+#define MEMO_GENERATION 2
 
 /**
  * Test that the qemu binary (or wrapper) runs, and do C<qemu -help>
@@ -152,8 +170,16 @@ guestfs_int_test_qemu (guestfs_h *g)
     r = qemu_fields[i].read_cache (g, data, filename);
     if (r == -1)
       goto error;
-    if (r == 0) /* cache gone, maybe deleted by the tmp cleaner */
+    if (r == 0) {
+      /* Cache gone, maybe deleted by the tmp cleaner, so we must run
+       * the full tests.  We will have a partially filled qemu_data
+       * structure.  The safest way to deal with that is to free
+       * it and start again.
+       */
+      guestfs_int_free_qemu_data (data);
+      data = safe_calloc (g, 1, sizeof *data);
       goto do_test;
+    }
   }
 
   goto out;
@@ -186,6 +212,8 @@ guestfs_int_test_qemu (guestfs_h *g)
  out:
   /* Derived fields. */
   parse_qemu_version (g, data->qemu_help, &data->qemu_version);
+  parse_json (g, data->qmp_commands, &data->qmp_commands_tree);
+  parse_json (g, data->qmp_schema, &data->qmp_schema_tree);
 
   return data;
 
@@ -276,6 +304,46 @@ write_cache_qemu_devices (guestfs_h *g, const struct
qemu_data *data,
 }
 
 static int
+test_qmp_commands (guestfs_h *g, struct qemu_data *data)
+{
+  return generic_qmp_test (g, data, "query-commands",
&data->qmp_commands);
+}
+
+static int
+read_cache_qmp_commands (guestfs_h *g, struct qemu_data *data,
+                         const char *filename)
+{
+  return generic_read_cache (g, filename, &data->qmp_commands);
+}
+
+static int
+write_cache_qmp_commands (guestfs_h *g, const struct qemu_data *data,
+                          const char *filename)
+{
+  return generic_write_cache (g, filename, data->qmp_commands);
+}
+
+static int
+test_qmp_schema (guestfs_h *g, struct qemu_data *data)
+{
+  return generic_qmp_test (g, data, "query-qmp-schema",
&data->qmp_schema);
+}
+
+static int
+read_cache_qmp_schema (guestfs_h *g, struct qemu_data *data,
+                       const char *filename)
+{
+  return generic_read_cache (g, filename, &data->qmp_schema);
+}
+
+static int
+write_cache_qmp_schema (guestfs_h *g, const struct qemu_data *data,
+                        const char *filename)
+{
+  return generic_write_cache (g, filename, data->qmp_schema);
+}
+
+static int
 read_cache_qemu_stat (guestfs_h *g, struct qemu_data *data,
                       const char *filename)
 {
@@ -338,6 +406,27 @@ parse_qemu_version (guestfs_h *g, const char *qemu_help,
 }
 
 /**
+ * Parse the json output from QMP.  But don't fail if parsing
+ * is not possible.
+ */
+static void
+parse_json (guestfs_h *g, const char *json, yajl_val *treep)
+{
+  char parse_error[256] = "";
+
+  if (!json)
+    return;
+
+  *treep = yajl_tree_parse (json, parse_error, sizeof parse_error);
+  if (*treep == NULL) {
+    if (strlen (parse_error) > 0)
+      debug (g, "QMP parse error: %s (ignored)", parse_error);
+    else
+      debug (g, "QMP unknown parse error (ignored)");
+  }
+}
+
+/**
  * Generic functions for reading and writing the cache files, used
  * where we are just reading and writing plain text strings.
  */
@@ -368,6 +457,86 @@ generic_write_cache (guestfs_h *g, const char *filename,
const char *str)
   return 0;
 }
 
+/**
+ * Run a generic QMP test on the QEMU binary.
+ */
+static int
+generic_qmp_test (guestfs_h *g, struct qemu_data *data,
+                  const char *qmp_command,
+                  char **outp)
+{
+  CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
+  int r, fd;
+  CLEANUP_FCLOSE FILE *fp = NULL;
+  CLEANUP_FREE char *line = NULL;
+  size_t allocsize = 0;
+  ssize_t len;
+
+  guestfs_int_cmd_add_string_unquoted (cmd, "echo ");
+  /* QMP is modal.  You have to send the qmp_capabilities command first. */
+  guestfs_int_cmd_add_string_unquoted (cmd, "'{ \"execute\":
\"qmp_capabilities\" }' ");
+  guestfs_int_cmd_add_string_unquoted (cmd, "'{ \"execute\":
\"");
+  guestfs_int_cmd_add_string_unquoted (cmd, qmp_command);
+  guestfs_int_cmd_add_string_unquoted (cmd, "\" }' ");
+  /* Exit QEMU after sending the commands. */
+  guestfs_int_cmd_add_string_unquoted (cmd, "'{ \"execute\":
\"quit\" }' ");
+  guestfs_int_cmd_add_string_unquoted (cmd, " | ");
+  guestfs_int_cmd_add_string_quoted (cmd, g->hv);
+  guestfs_int_cmd_add_string_unquoted (cmd, " -display none");
+  guestfs_int_cmd_add_string_unquoted (cmd, " -machine ");
+  guestfs_int_cmd_add_string_quoted (cmd,
+#ifdef MACHINE_TYPE
+                                     MACHINE_TYPE ","
+#endif
+                                     "accel=kvm:tcg");
+  guestfs_int_cmd_add_string_unquoted (cmd, " -qmp stdio");
+  guestfs_int_cmd_clear_capture_errors (cmd);
+
+  fd = guestfs_int_cmd_pipe_run (cmd, "r");
+  if (fd == -1)
+    return -1;
+
+  /* Read the output line by line.  We expect to see:
+   * line 1: {"QMP": {"version": ... } }   # greeting from
QMP
+   * line 2: {"return": {}}                # output from
qmp_capabilities
+   * line 3: {"return": ... }              # the data from our
qmp_command
+   * line 4: {"return": {}}                # output from quit
+   * line 5: {"timestamp": ...}            # shutdown event
+   */
+  fp = fdopen (fd, "r");        /* this will close (fd) at end of
scope */
+  if (fp == NULL) {
+    perrorf (g, "fdopen");
+    return -1;
+  }
+  len = getline (&line, &allocsize, fp); /* line 1 */
+  if (len == -1 || strstr (line, "\"QMP\"") == NULL) {
+  parse_failure:
+    debug (g, "did not understand QMP monitor output from %s
(ignored)",
+           g->hv);
+    /* QMP tests are optional, don't fail if we cannot parse the output. */
+    return 0;
+  }
+  len = getline (&line, &allocsize, fp); /* line 2 */
+  if (len == -1 || strstr (line, "\"return\"") == NULL)
+    goto parse_failure;
+  len = getline (&line, &allocsize, fp); /* line 3 */
+  if (len == -1 || strstr (line, "\"return\"") == NULL)
+    goto parse_failure;
+  *outp = safe_strdup (g, line);
+  /* The other lines we don't care about, so finish parsing here. */
+  ignore_value (getline (&line, &allocsize, fp)); /* line 4 */
+  ignore_value (getline (&line, &allocsize, fp)); /* line 5 */
+
+  r = guestfs_int_cmd_pipe_wait (cmd);
+  /* QMP tests are optional, don't fail if the tests fail. */
+  if (r == -1 || !WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    debug (g, "%s wait failed or unexpected exit status (ignored)",
g->hv);
+    return 0;
+  }
+
+  return 0;
+}
+
 static void
 read_all (guestfs_h *g, void *retv, const char *buf, size_t len)
 {
@@ -754,6 +923,10 @@ guestfs_int_free_qemu_data (struct qemu_data *data)
   if (data) {
     free (data->qemu_help);
     free (data->qemu_devices);
+    free (data->qmp_commands);
+    free (data->qmp_schema);
+    yajl_tree_free (data->qmp_commands_tree);
+    yajl_tree_free (data->qmp_schema_tree);
     free (data);
   }
 }
-- 
2.13.2

QEMU >= 2.10 started to do mandatory locking.  This checks the QMP
schema to see if we are using that version of qemu.  (Note it is
sometimes disabled in downstream builds, and it was also enabled in
upstream prereleases with version 2.9.9x, so we cannot just check the
version number).
---
 lib/guestfs-internal.h |  1 +
 lib/qemu.c             | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/lib/guestfs-internal.h b/lib/guestfs-internal.h
index 2ca258cb3..ab839de4d 100644
--- a/lib/guestfs-internal.h
+++ b/lib/guestfs-internal.h
@@ -983,6 +983,7 @@ extern struct qemu_data *guestfs_int_test_qemu (guestfs_h
*g);
 extern struct version guestfs_int_qemu_version (guestfs_h *g, struct qemu_data
*);
 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_mandatory_locking (guestfs_h *g, const struct
qemu_data *data);
 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 version *qemu_version);
 extern char *guestfs_int_qemu_escape_param (guestfs_h *g, const char *param);
diff --git a/lib/qemu.c b/lib/qemu.c
index 34775041f..32f930259 100644
--- a/lib/qemu.c
+++ b/lib/qemu.c
@@ -577,6 +577,66 @@ guestfs_int_qemu_supports_device (guestfs_h *g,
   return strstr (data->qemu_devices, device_name) != NULL;
 }
 
+/* GCC can't work out that the YAJL_IS_<foo> test is sufficient to
+ * ensure that YAJL_GET_<foo> later doesn't return NULL.
+ */
+#pragma GCC diagnostic push
+#if defined(__GNUC__) && __GNUC__ >= 6 /* gcc >= 6 */
+#pragma GCC diagnostic ignored "-Wnull-dereference"
+#endif
+
+/**
+ * Test if the QMP schema contains a particular C<(key, value)> pair
+ * (anywhere).  QMP is almost impossible to parse sanely so this is
+ * the minimum we need to detect if C<"locking"> is supported.
+ */
+static int
+qmp_schema_contains (yajl_val tree, const char *key, const char *value)
+{
+  size_t i;
+
+  if (YAJL_IS_OBJECT (tree)) {
+    for (i = 0; i < YAJL_GET_OBJECT(tree)->len; ++i) {
+      const char *k;
+      yajl_val v;
+
+      k = YAJL_GET_OBJECT(tree)->keys[i];
+      v = YAJL_GET_OBJECT(tree)->values[i];
+
+      if (STREQ (k, key) &&
+          YAJL_IS_STRING (v) && STREQ (YAJL_GET_STRING (v), value))
+          return 1;
+      if (qmp_schema_contains (v, key, value))
+        return 1;
+    }
+  }
+  else if (YAJL_IS_ARRAY (tree)) {
+    for (i = 0; i < YAJL_GET_ARRAY(tree)->len; ++i) {
+      yajl_val v;
+
+      v = YAJL_GET_ARRAY(tree)->values[i];
+
+      if (qmp_schema_contains (v, key, value))
+        return 1;
+    }
+  }
+
+  return 0;
+}
+
+#pragma GCC diagnostic pop
+
+/**
+ * Test if the qemu binary uses mandatory file locking, added in
+ * QEMU >= 2.10 (but sometimes disabled).
+ */
+int
+guestfs_int_qemu_mandatory_locking (guestfs_h *g,
+                                    const struct qemu_data *data)
+{
+  return qmp_schema_contains (data->qmp_schema_tree, "name",
"locking");
+}
+
 /**
  * Escape a qemu parameter.
  *
-- 
2.13.2

---
 lib/launch-direct.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/launch-direct.c b/lib/launch-direct.c
index 9f85013f9..00cb25077 100644
--- a/lib/launch-direct.c
+++ b/lib/launch-direct.c
@@ -66,6 +66,7 @@ struct backend_direct_data {
   pid_t recoverypid;            /* Recovery process PID. */
 
   struct version qemu_version;  /* qemu version (0 if unable to parse). */
+  int qemu_mandatory_locking;   /* qemu >= 2.10 does mandatory locking */
   struct qemu_data *qemu_data;  /* qemu -help output etc. */
 
   char guestfsd_sock[UNIX_PATH_MAX]; /* Path to daemon socket. */
@@ -255,11 +256,13 @@ add_drive_standard_params (guestfs_h *g, struct
backend_direct_data *data,
   }
   else {
     /* Writable qcow2 overlay on top of read-only drive. */
-    append_list_format ("file=%s", drv->overlay);
+    append_list_format ("file.file.filename=%s", drv->overlay);
+    append_list ("file.driver=qcow2");
     append_list ("cache=unsafe");
-    append_list ("format=qcow2");
     if (drv->disk_label)
       append_list_format ("serial=%s", drv->disk_label);
+    if (data->qemu_mandatory_locking)
+      append_list ("file.backing.file.locking=off");
   }
 
   append_list_format ("id=hd%zu", i);
@@ -408,6 +411,10 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
     data->qemu_version = guestfs_int_qemu_version (g, data->qemu_data);
     debug (g, "qemu version: %d.%d",
            data->qemu_version.v_major, data->qemu_version.v_minor);
+    data->qemu_mandatory_locking +      guestfs_int_qemu_mandatory_locking
(g, data->qemu_data);
+    debug (g, "qemu mandatory locking: %s",
+           data->qemu_mandatory_locking ? "yes" : "no");
   }
 
   /* Using virtio-serial, we need to create a local Unix domain socket
-- 
2.13.2

On Tuesday, 12 September 2017 14:29:13 CEST Richard W.M. Jones
wrote:
> +/**
> + * Generic functions for reading and writing the cache files, used
> + * where we are just reading and writing plain text strings.
> + */
> +static int
> +generic_read_cache (guestfs_h *g, const char *filename, char **strp)
> +{
> +  if (access (filename, R_OK) == -1 && errno == ENOENT)
> +    return 0;                   /* no cache, run the test instead */
This will go ahead if access() failed for any other error though;
IMHO a better check could be:

  if (access (filename, R_OK) == -1) {
    if (errno == ENOENT)
      return 0;                   /* no cache, run the test instead */
    perrorf (g, "access: %s", filename);
    return -1;
  }
> +static int
> +generic_write_cache (guestfs_h *g, const char *filename, const char *str)
> +{
> +  CLEANUP_FCLOSE FILE *fp = fopen (filename, "w");
> +  if (fp == NULL) {
> +    perrorf (g, "%s", filename);
> +    return -1;
> +  }
> +
> +  if (fprintf (fp, "%s", str) == -1) {
> +    perrorf (g, "%s: write", filename);
> +    return -1;
> +  }
> +
> +  return 0;
> +}
While this is the same code already used, IMHO it would make more sense
to use open/write directly (since we have a buffer already, it will be
faster than using sprintf); there are snippets that call write() in a
loop until the whole buffer is written in different parts of the
library, so factorizing them could help.

Thanks,
-- 
Pino Toscano

On Tuesday, 12 September 2017 14:29:14 CEST Richard W.M. Jones
wrote:
> This adds two extra tests using QMP (the QEMU Monitor Protocol).  This
> allows us to get extra information about the qemu binary beyond what
> is available from the version number or ‘qemu -help’.
> ---
>  lib/qemu.c | 177
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 175 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/qemu.c b/lib/qemu.c
> index bdd9947a8..34775041f 100644
> --- a/lib/qemu.c
> +++ b/lib/qemu.c
> @@ -37,6 +37,8 @@
>  
>  #include <libxml/uri.h>
>  
> +#include <yajl/yajl_tree.h>
> +
>  #include "ignore-value.h"
>  
>  #include "guestfs.h"
> @@ -50,9 +52,13 @@ struct qemu_data {
>  
>    char *qemu_help;              /* Output of qemu -help. */
>    char *qemu_devices;           /* Output of qemu -device ? */
> +  char *qmp_commands;           /* Output of QMP query-commands. */
> +  char *qmp_schema;             /* Output of QMP query-qmp-schema. */
Considering we keep them as parsed by yajl, wouldn't it better to avoid
keeping the raw outputs too?

Also the result of query-commands does not seem to be used in other
patches this series.  Is it going to be used in another followup
series?

-- 
Pino Toscano

On Tuesday, 12 September 2017 14:29:15 CEST Richard W.M. Jones
wrote:
> +/**
> + * Test if the QMP schema contains a particular C<(key, value)> pair
> + * (anywhere).  QMP is almost impossible to parse sanely so this is
> + * the minimum we need to detect if C<"locking"> is
supported.
> + */
> +static int
> +qmp_schema_contains (yajl_val tree, const char *key, const char *value)
> +{
> +  size_t i;
> +
> +  if (YAJL_IS_OBJECT (tree)) {
> +    for (i = 0; i < YAJL_GET_OBJECT(tree)->len; ++i) {
> +      const char *k;
> +      yajl_val v;
> +
> +      k = YAJL_GET_OBJECT(tree)->keys[i];
> +      v = YAJL_GET_OBJECT(tree)->values[i];
> +
> +      if (STREQ (k, key) &&
> +          YAJL_IS_STRING (v) && STREQ (YAJL_GET_STRING (v),
value))
> +          return 1;
> +      if (qmp_schema_contains (v, key, value))
> +        return 1;
> +    }
> +  }
> +  else if (YAJL_IS_ARRAY (tree)) {
> +    for (i = 0; i < YAJL_GET_ARRAY(tree)->len; ++i) {
> +      yajl_val v;
> +
> +      v = YAJL_GET_ARRAY(tree)->values[i];
> +
> +      if (qmp_schema_contains (v, key, value))
> +        return 1;
> +    }
> +  }
> +
> +  return 0;
> +}
This can easily exhaust the stack when trying to parse some
badly-formatted JSON snippet; considering the user can change the
hypervisor, then it can easily become a mild local DoS.  A simple
workaround can be to limit the recursion depth, like done in the OCaml
binding to yajl_parse_tree (builder/yajl-c.c).

If the path to the key is known in advance (e.g. root object ->
"return" key -> list -> object with "name" key equal
to "locking"),
then IMHO would be better to just look for that.

-- 
Pino Toscano

On Tuesday, 12 September 2017 14:29:16 CEST Richard W.M. Jones
wrote:
> @@ -255,11 +256,13 @@ add_drive_standard_params (guestfs_h *g, struct
backend_direct_data *data,
>    }
>    else {
>      /* Writable qcow2 overlay on top of read-only drive. */
> -    append_list_format ("file=%s", drv->overlay);
> +    append_list_format ("file.file.filename=%s",
drv->overlay);
> +    append_list ("file.driver=qcow2");
>      append_list ("cache=unsafe");
> -    append_list ("format=qcow2");
>      if (drv->disk_label)
>        append_list_format ("serial=%s", drv->disk_label);
> +    if (data->qemu_mandatory_locking)
> +      append_list ("file.backing.file.locking=off");
>    }
The same ought to be done for the appliance drive too.

-- 
Pino Toscano