Libguestfs - Mar 2014 - [PATCH 0/3] Add discard support.

These patches contain the beginnings of discard (a.k.a. trim or unmap)
support.  This will allow us to change virt-sparsify to work on disk
images in-place (instead of using slow & inefficient copying).

The approach used is to add an optional 'discard' parameter to
add-drive.  It has 3 possible settings:

 - 'disable' : the default, no discard is done
 - 'besteffort' : try to enable it, but don't fail if it's not
supported
 - 'enable' : enable it, and fail if it is not supported

I'm not sure if we should wait for post-1.26 before adding discard
support.  It depends if the interface above is suitable or if we'll
have to change it later.  However I discussed how to support this at
DevConf and this approach was generally agreed.

Rich.

This is just rearranging the data between structs.  There should be no
functional change.
---
 src/launch-libvirt.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index fd6e6d2..d86e2d2 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -106,6 +106,7 @@ struct backend_libvirt_data {
   char *selinux_imagelabel;
   bool selinux_norelabel_disks;
   char name[DOMAIN_NAME_LEN];   /* random name */
+  bool is_kvm;                  /* false = qemu, true = kvm (from
capabilities)*/
 };
 
 /* Parameters passed to construct_libvirt_xml and subfunctions.  We
@@ -122,12 +123,11 @@ struct libvirt_xml_params {
   char guestfsd_path[UNIX_PATH_MAX]; /* paths to sockets */
   char console_path[UNIX_PATH_MAX];
   bool enable_svirt;            /* false if we decided to disable sVirt */
-  bool is_kvm;                  /* false = qemu, true = kvm */
   bool current_proc_is_root;    /* true = euid is root */
 };
 
-static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct libvirt_xml_params *params);
-static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params);
+static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct backend_libvirt_data *data);
+static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params);
 static void debug_appliance_permissions (guestfs_h *g);
 static void debug_socket_permissions (guestfs_h *g);
 static void libvirt_error (guestfs_h *g, const char *fs, ...)
__attribute__((format (printf,2,3)));
@@ -299,7 +299,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   if (g->verbose)
     guestfs___print_timestamped_message (g, "parsing capabilities
XML");
 
-  if (parse_capabilities (g, capabilities_xml, &params) == -1)
+  if (parse_capabilities (g, capabilities_xml, data) == -1)
     goto cleanup;
 
   /* Locate and/or build the appliance. */
@@ -434,7 +434,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
   guestfs___drive_name (params.appliance_index, &params.appliance_dev[7]);
   params.enable_svirt = ! is_custom_hv (g);
 
-  xml = construct_libvirt_xml (g, &params);
+  xml = construct_libvirt_xml (g, data, &params);
   if (!xml)
     goto cleanup;
 
@@ -575,7 +575,7 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
 
 static int
 parse_capabilities (guestfs_h *g, const char *capabilities_xml,
-                    struct libvirt_xml_params *params)
+                    struct backend_libvirt_data *data)
 {
   CLEANUP_XMLFREEDOC xmlDocPtr doc = NULL;
   CLEANUP_XMLXPATHFREECONTEXT xmlXPathContextPtr xpathCtx = NULL;
@@ -654,9 +654,9 @@ parse_capabilities (guestfs_h *g, const char
*capabilities_xml,
   force_tcg = guestfs___get_backend_setting_bool (g, "force_tcg");
 
   if (!force_tcg)
-    params->is_kvm = seen_kvm;
+    data->is_kvm = seen_kvm;
   else
-    params->is_kvm = 0;
+    data->is_kvm = 0;
 
   return 0;
 }
@@ -779,10 +779,10 @@ debug_socket_permissions (guestfs_h *g)
   }
 }
 
-static int construct_libvirt_xml_domain (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
+static int construct_libvirt_xml_domain (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params,
xmlTextWriterPtr xo);
 static int construct_libvirt_xml_name (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
-static int construct_libvirt_xml_cpu (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
-static int construct_libvirt_xml_boot (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
+static int construct_libvirt_xml_cpu (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params,
xmlTextWriterPtr xo);
+static int construct_libvirt_xml_boot (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params,
xmlTextWriterPtr xo);
 static int construct_libvirt_xml_seclabel (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_lifecycle (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_devices (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
@@ -872,7 +872,9 @@ static int construct_libvirt_xml_appliance (guestfs_h *g,
const struct libvirt_x
              __FILE__, __LINE__, (fn));
 
 static xmlChar *
-construct_libvirt_xml (guestfs_h *g, const struct libvirt_xml_params *params)
+construct_libvirt_xml (guestfs_h *g,
+                       const struct backend_libvirt_data *data,
+                       const struct libvirt_xml_params *params)
 {
   xmlChar *ret = NULL;
   CLEANUP_XMLBUFFERFREE xmlBufferPtr xb = NULL;
@@ -905,7 +907,7 @@ construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params)
     return NULL;
   }
 
-  if (construct_libvirt_xml_domain (g, params, xo) == -1)
+  if (construct_libvirt_xml_domain (g, data, params, xo) == -1)
     return NULL;
 
   if (xmlTextWriterEndDocument (xo) == -1) {
@@ -925,19 +927,20 @@ construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params)
 
 static int
 construct_libvirt_xml_domain (guestfs_h *g,
+                              const struct backend_libvirt_data *data,
                               const struct libvirt_xml_params *params,
                               xmlTextWriterPtr xo)
 {
   start_element ("domain") {
-    attribute ("type", params->is_kvm ? "kvm" :
"qemu");
+    attribute ("type", data->is_kvm ? "kvm" :
"qemu");
     attribute_ns ("xmlns", "qemu", NULL,
                   "http://libvirt.org/schemas/domain/qemu/1.0");
 
     if (construct_libvirt_xml_name (g, params, xo) == -1)
       return -1;
-    if (construct_libvirt_xml_cpu (g, params, xo) == -1)
+    if (construct_libvirt_xml_cpu (g, data, params, xo) == -1)
       return -1;
-    if (construct_libvirt_xml_boot (g, params, xo) == -1)
+    if (construct_libvirt_xml_boot (g, data, params, xo) == -1)
       return -1;
     if (construct_libvirt_xml_seclabel (g, params, xo) == -1)
       return -1;
@@ -968,6 +971,7 @@ construct_libvirt_xml_name (guestfs_h *g,
 /* CPU and memory features. */
 static int
 construct_libvirt_xml_cpu (guestfs_h *g,
+                           const struct backend_libvirt_data *data,
                            const struct libvirt_xml_params *params,
                            xmlTextWriterPtr xo)
 {
@@ -987,7 +991,7 @@ construct_libvirt_xml_cpu (guestfs_h *g,
    * Only do this with KVM.  It is broken in subtle ways on TCG, and
    * fairly pointless anyway.
    */
-  if (params->is_kvm) {
+  if (data->is_kvm) {
     start_element ("cpu") {
       attribute ("mode", "host-passthrough");
       start_element ("model") {
@@ -1031,6 +1035,7 @@ construct_libvirt_xml_cpu (guestfs_h *g,
 /* Boot parameters. */
 static int
 construct_libvirt_xml_boot (guestfs_h *g,
+                            const struct backend_libvirt_data *data,
                             const struct libvirt_xml_params *params,
                             xmlTextWriterPtr xo)
 {
@@ -1039,7 +1044,7 @@ construct_libvirt_xml_boot (guestfs_h *g,
 
   /* Linux kernel command line. */
   flags = 0;
-  if (!params->is_kvm)
+  if (!data->is_kvm)
     flags |= APPLIANCE_COMMAND_LINE_IS_TCG;
   cmdline = guestfs___appliance_command_line (g, params->appliance_dev,
flags);
 
-- 
1.8.5.3

This adds a discard parameter to the guestfs_add_drive_opts which
approximately maps to the discard=ignore|unmap parameter supported by
qemu.

If discard is set to "enable" then we force discard=unmap (and try to
fail if it is not possible).  If discard is set to the more useful
"besteffort" option, then we enable discard if possible.  The default
is "disable".
---
 generator/actions.ml   |  32 +++++++++++++-
 src/drives.c           | 107 +++++++++++++++++++++++++++++++----------------
 src/guestfs-internal.h |   9 ++++
 src/launch-direct.c    | 110 ++++++++++++++++++++++++++++++++++++++++++++++++-
 src/launch-libvirt.c   |  96 ++++++++++++++++++++++++++++++++++++++----
 src/launch-uml.c       |   6 +++
 6 files changed, 316 insertions(+), 44 deletions(-)

diff --git a/generator/actions.ml b/generator/actions.ml
index a6ef194..b25194c 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -1286,7 +1286,7 @@ not all belong to a single logical operating system
 
   { defaults with
     name = "add_drive";
-    style = RErr, [String "filename"], [OBool "readonly";
OString "format"; OString "iface"; OString "name";
OString "label"; OString "protocol"; OStringList
"server"; OString "username"; OString "secret";
OString "cachemode"];
+    style = RErr, [String "filename"], [OBool "readonly";
OString "format"; OString "iface"; OString "name";
OString "label"; OString "protocol"; OStringList
"server"; OString "username"; OString "secret";
OString "cachemode"; OString "discard"];
     once_had_no_optargs = true;
     blocking = false;
     fish_alias = ["add"];
@@ -1504,6 +1504,36 @@ for scratch or temporary disks.
 
 =back
 
+=item C<discard>
+
+Enable or disable discard (a.k.a. trim or unmap) support on this
+drive.  If enabled, operations such as C<guestfs_fstrim> will be able
+to discard / make thin / punch holes in the underlying host file
+or device.
+
+Possible discard settings are:
+
+=over 4
+
+=item C<discard = \"disable\">
+
+Disable discard support.  This is the default.
+
+=item C<discard = \"enable\">
+
+Enable discard support.  Fail if discard is not possible.
+
+=item C<discard = \"besteffort\">
+
+Enable discard support if possible, but don't fail if it is not
+supported.
+
+Since not all backends and not all underlying systems support
+discard, this is a good choice if you want to use discard if
+possible, but don't mind if it doesn't work.
+
+=back
+
 =back" };
 
   { defaults with
diff --git a/src/drives.c b/src/drives.c
index 2c85b52..68e37f7 100644
--- a/src/drives.c
+++ b/src/drives.c
@@ -115,7 +115,8 @@ static struct drive *
 create_drive_file (guestfs_h *g, const char *path,
                    bool readonly, const char *format,
                    const char *iface, const char *name,
-                   const char *disk_label, const char *cachemode)
+                   const char *disk_label, const char *cachemode,
+                   enum discard discard)
 {
   struct drive *drv = safe_calloc (g, 1, sizeof *drv);
 
@@ -128,6 +129,7 @@ create_drive_file (guestfs_h *g, const char *path,
   drv->name = name ? safe_strdup (g, name) : NULL;
   drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
   drv->cachemode = cachemode ? safe_strdup (g, cachemode) : NULL;
+  drv->discard = discard;
 
   if (readonly) {
     if (create_overlay (g, drv) == -1) {
@@ -151,7 +153,8 @@ create_drive_non_file (guestfs_h *g,
                        const char *username, const char *secret,
                        bool readonly, const char *format,
                        const char *iface, const char *name,
-                       const char *disk_label, const char *cachemode)
+                       const char *disk_label, const char *cachemode,
+                       enum discard discard)
 {
   struct drive *drv = safe_calloc (g, 1, sizeof *drv);
 
@@ -168,6 +171,7 @@ create_drive_non_file (guestfs_h *g,
   drv->name = name ? safe_strdup (g, name) : NULL;
   drv->disk_label = disk_label ? safe_strdup (g, disk_label) : NULL;
   drv->cachemode = cachemode ? safe_strdup (g, cachemode) : NULL;
+  drv->discard = discard;
 
   if (readonly) {
     if (create_overlay (g, drv) == -1) {
@@ -191,7 +195,8 @@ create_drive_curl (guestfs_h *g,
                    const char *username, const char *secret,
                    bool readonly, const char *format,
                    const char *iface, const char *name,
-                   const char *disk_label, const char *cachemode)
+                   const char *disk_label, const char *cachemode,
+                   enum discard discard)
 {
   if (secret != NULL) {
     error (g, _("curl: you cannot specify a secret with this
protocol"));
@@ -223,7 +228,7 @@ create_drive_curl (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 static struct drive *
@@ -233,7 +238,8 @@ create_drive_gluster (guestfs_h *g,
                       const char *username, const char *secret,
                       bool readonly, const char *format,
                       const char *iface, const char *name,
-                      const char *disk_label, const char *cachemode)
+                      const char *disk_label, const char *cachemode,
+                      enum discard discard)
 {
   if (username != NULL) {
     error (g, _("gluster: you cannot specify a username with this
protocol"));
@@ -263,7 +269,7 @@ create_drive_gluster (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 static int
@@ -285,7 +291,8 @@ create_drive_nbd (guestfs_h *g,
                   const char *username, const char *secret,
                   bool readonly, const char *format,
                   const char *iface, const char *name,
-                  const char *disk_label, const char *cachemode)
+                  const char *disk_label, const char *cachemode,
+                  enum discard discard)
 {
   if (username != NULL) {
     error (g, _("nbd: you cannot specify a username with this
protocol"));
@@ -308,7 +315,7 @@ create_drive_nbd (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 static struct drive *
@@ -318,7 +325,8 @@ create_drive_rbd (guestfs_h *g,
                   const char *username, const char *secret,
                   bool readonly, const char *format,
                   const char *iface, const char *name,
-                  const char *disk_label, const char *cachemode)
+                  const char *disk_label, const char *cachemode,
+                  enum discard discard)
 {
   size_t i;
 
@@ -348,7 +356,7 @@ create_drive_rbd (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 static struct drive *
@@ -358,7 +366,8 @@ create_drive_sheepdog (guestfs_h *g,
                        const char *username, const char *secret,
                        bool readonly, const char *format,
                        const char *iface, const char *name,
-                       const char *disk_label, const char *cachemode)
+                       const char *disk_label, const char *cachemode,
+                       enum discard discard)
 {
   size_t i;
 
@@ -397,7 +406,7 @@ create_drive_sheepdog (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 static struct drive *
@@ -407,7 +416,8 @@ create_drive_ssh (guestfs_h *g,
                   const char *username, const char *secret,
                   bool readonly, const char *format,
                   const char *iface, const char *name,
-                  const char *disk_label, const char *cachemode)
+                  const char *disk_label, const char *cachemode,
+                  enum discard discard)
 {
   if (secret != NULL) {
     error (g, _("ssh: you cannot specify a secret with this
protocol"));
@@ -444,7 +454,7 @@ create_drive_ssh (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 static struct drive *
@@ -454,7 +464,8 @@ create_drive_iscsi (guestfs_h *g,
                     const char *username, const char *secret,
                     bool readonly, const char *format,
                     const char *iface, const char *name,
-                    const char *disk_label, const char *cachemode)
+                    const char *disk_label, const char *cachemode,
+                    enum discard discard)
 {
   if (username != NULL) {
     error (g, _("iscsi: you cannot specify a username with this
protocol"));
@@ -491,7 +502,7 @@ create_drive_iscsi (guestfs_h *g,
                                 servers, nr_servers, exportname,
                                 username, secret,
                                 readonly, format, iface, name, disk_label,
-                                cachemode);
+                                cachemode, discard);
 }
 
 /* Traditionally you have been able to use /dev/null as a filename, as
@@ -528,14 +539,15 @@ create_drive_dev_null (guestfs_h *g, bool readonly, const
char *format,
     return NULL;
 
   return create_drive_file (g, tmpfile, readonly, format, iface, name,
-                            disk_label, 0);
+                            disk_label, NULL, discard_disable);
 }
 
 static struct drive *
 create_drive_dummy (guestfs_h *g)
 {
   /* A special drive struct that is used as a dummy slot for the appliance. */
-  return create_drive_file (g, "", 0, NULL, NULL, NULL, NULL, 0);
+  return create_drive_file (g, "", 0, NULL, NULL, NULL, NULL, NULL,
+                            discard_disable);
 }
 
 static void
@@ -563,8 +575,8 @@ free_drive_struct (struct drive *drv)
   free (drv);
 }
 
-static const char *
-protocol_to_string (enum drive_protocol protocol)
+const char *
+guestfs___drive_protocol_to_string (enum drive_protocol protocol)
 {
   switch (protocol) {
   case drive_protocol_file: return "file";
@@ -590,12 +602,12 @@ static char *
 drive_to_string (guestfs_h *g, const struct drive *drv)
 {
   return safe_asprintf
-    (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s",
+    (g, "%s%s%s%s protocol=%s%s%s%s%s%s%s%s%s%s",
      drv->src.u.path,
      drv->readonly ? " readonly" : "",
      drv->src.format ? " format=" : "",
      drv->src.format ? : "",
-     protocol_to_string (drv->src.protocol),
+     guestfs___drive_protocol_to_string (drv->src.protocol),
      drv->iface ? " iface=" : "",
      drv->iface ? : "",
      drv->name ? " name=" : "",
@@ -603,7 +615,9 @@ drive_to_string (guestfs_h *g, const struct drive *drv)
      drv->disk_label ? " label=" : "",
      drv->disk_label ? : "",
      drv->cachemode ? " cache=" : "",
-     drv->cachemode ? : "");
+     drv->cachemode ? : "",
+     drv->discard == discard_disable ? "" :
+     drv->discard == discard_enable ? " discard=enable" : "
discard=besteffort");
 }
 
 /* Add struct drive to the g->drives vector at the given index. */
@@ -824,6 +838,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
   const char *username;
   const char *secret;
   const char *cachemode;
+  enum discard discard;
   struct drive *drv;
   size_t i, drv_index;
 
@@ -852,6 +867,28 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
   cachemode = optargs->bitmask &
GUESTFS_ADD_DRIVE_OPTS_CACHEMODE_BITMASK
     ? optargs->cachemode : NULL;
 
+  if (optargs->bitmask & GUESTFS_ADD_DRIVE_OPTS_DISCARD_BITMASK) {
+    if (STREQ (optargs->discard, "disable"))
+      discard = discard_disable;
+    else if (STREQ (optargs->discard, "enable"))
+      discard = discard_enable;
+    else if (STREQ (optargs->discard, "besteffort"))
+      discard = discard_besteffort;
+    else {
+      error (g, _("discard parameter must be 'disable',
'enable' or 'besteffort'"));
+      free_drive_servers (servers, nr_servers);
+      return -1;
+    }
+  }
+  else
+    discard = discard_disable;
+
+  if (readonly && discard == discard_enable) {
+    error (g, _("discard support cannot be enabled on read-only
drives"));
+    free_drive_servers (servers, nr_servers);
+    return -1;
+  }
+
   if (format && !valid_format_iface (format)) {
     error (g, _("%s parameter is empty or contains disallowed
characters"),
            "format");
@@ -904,7 +941,7 @@ guestfs__add_drive_opts (guestfs_h *g, const char *filename,
       }
 
       drv = create_drive_file (g, filename, readonly, format, iface, name,
-                               disk_label, cachemode);
+                               disk_label, cachemode, discard);
     }
   }
   else if (STREQ (protocol, "ftp")) {
@@ -912,71 +949,71 @@ guestfs__add_drive_opts (guestfs_h *g, const char
*filename,
                              servers, nr_servers, filename,
                              username, secret,
                              readonly, format, iface, name,
-                             disk_label, cachemode);
+                             disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "ftps")) {
     drv = create_drive_curl (g, drive_protocol_ftps,
                              servers, nr_servers, filename,
                              username, secret,
                              readonly, format, iface, name,
-                             disk_label, cachemode);
+                             disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "gluster")) {
     drv = create_drive_gluster (g, servers, nr_servers, filename,
                                 username, secret,
                                 readonly, format, iface, name,
-                                disk_label, cachemode);
+                                disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "http")) {
     drv = create_drive_curl (g, drive_protocol_http,
                              servers, nr_servers, filename,
                              username, secret,
                              readonly, format, iface, name,
-                             disk_label, cachemode);
+                             disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "https")) {
     drv = create_drive_curl (g, drive_protocol_https,
                              servers, nr_servers, filename,
                              username, secret,
                              readonly, format, iface, name,
-                             disk_label, cachemode);
+                             disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "iscsi")) {
     drv = create_drive_iscsi (g, servers, nr_servers, filename,
                               username, secret,
                               readonly, format, iface, name,
-                              disk_label, cachemode);
+                              disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "nbd")) {
     drv = create_drive_nbd (g, servers, nr_servers, filename,
                             username, secret,
                             readonly, format, iface, name,
-                            disk_label, cachemode);
+                            disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "rbd")) {
     drv = create_drive_rbd (g, servers, nr_servers, filename,
                             username, secret,
                             readonly, format, iface, name,
-                            disk_label, cachemode);
+                            disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "sheepdog")) {
     drv = create_drive_sheepdog (g, servers, nr_servers, filename,
                                  username, secret,
                                  readonly, format, iface, name,
-                                 disk_label, cachemode);
+                                 disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "ssh")) {
     drv = create_drive_ssh (g, servers, nr_servers, filename,
                             username, secret,
                             readonly, format, iface, name,
-                            disk_label, cachemode);
+                            disk_label, cachemode, discard);
   }
   else if (STREQ (protocol, "tftp")) {
     drv = create_drive_curl (g, drive_protocol_tftp,
                              servers, nr_servers, filename,
                              username, secret,
                              readonly, format, iface, name,
-                             disk_label, cachemode);
+                             disk_label, cachemode, discard);
   }
   else {
     error (g, _("unknown protocol '%s'"), protocol);
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 545146f..df481a7 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -231,6 +231,12 @@ struct drive_source {
   char *secret;
 };
 
+enum discard {
+  discard_disable = 0,
+  discard_enable = 1,
+  discard_besteffort = 2,
+};
+
 /* There is one 'struct drive' per drive, including hot-plugged drives.
*/
 struct drive {
   /* Original source of the drive, eg. file:..., http:... */
@@ -252,6 +258,7 @@ struct drive {
   char *name;
   char *disk_label;
   char *cachemode;
+  enum discard discard;
 };
 
 /* Extra hv parameters (from guestfs_config). */
@@ -713,6 +720,7 @@ extern size_t guestfs___checkpoint_drives (guestfs_h *g);
 extern void guestfs___rollback_drives (guestfs_h *g, size_t);
 extern void guestfs___add_dummy_appliance_drive (guestfs_h *g);
 extern void guestfs___free_drives (guestfs_h *g);
+extern const char *guestfs___drive_protocol_to_string (enum drive_protocol
protocol);
 
 /* appliance.c */
 extern int guestfs___build_appliance (guestfs_h *g, char **kernel, char **dtb,
char **initrd, char **appliance);
@@ -832,6 +840,7 @@ extern void guestfs___cleanup_cmd_close (struct command **);
 
 /* launch-direct.c */
 extern char *guestfs___drive_source_qemu_param (guestfs_h *g, const struct
drive_source *src);
+extern bool guestfs___discard_possible (guestfs_h *g, struct drive *drv, bool
qemu15, bool qemu16);
 
 /* utils.c */
 extern int guestfs___validate_guid (const char *);
diff --git a/src/launch-direct.c b/src/launch-direct.c
index 1e84061..584c4de 100644
--- a/src/launch-direct.c
+++ b/src/launch-direct.c
@@ -509,6 +509,29 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
     CLEANUP_FREE char *file = NULL, *escaped_file = NULL, *param = NULL;
 
     if (!drv->overlay) {
+      const char *discard_mode = "";
+      int major = data->qemu_version_major, minor =
data->qemu_version_minor;
+
+      switch (drv->discard) {
+      case discard_disable:
+        /* Since the default is always discard=ignore, don't specify it
+         * on the command line.  This also avoids unnecessary breakage
+         * with qemu < 1.5 which didn't have the option at all.
+         */
+        break;
+      case discard_enable:
+        if (!guestfs___discard_possible (g, drv, major, minor))
+          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))
+          discard_mode = ",discard=unmap";
+        break;
+      }
+
       /* Make the file= parameter. */
       file = guestfs___drive_source_qemu_param (g, &drv->src);
       escaped_file = qemu_escape_param (g, file);
@@ -517,10 +540,11 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
        * the if=... at the end.
        */
       param = safe_asprintf
-        (g, "file=%s%s,cache=%s%s%s%s%s,id=hd%zu",
+        (g, "file=%s%s,cache=%s%s%s%s%s%s,id=hd%zu",
          escaped_file,
          drv->readonly ? ",snapshot=on" : "",
          drv->cachemode ? drv->cachemode : "writeback",
+         discard_mode,
          drv->src.format ? ",format=" : "",
          drv->src.format ? drv->src.format : "",
          drv->disk_label ? ",serial=" : "",
@@ -1370,6 +1394,90 @@ guestfs___drive_source_qemu_param (guestfs_h *g, const
struct drive_source *src)
   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".
+ *
+ * qemu15: qemu >= 1.5.  This was the first version that supported
+ * the discard option on -drive at all.
+ *
+ * qemu16: qemu >= 1.6.  This was the first version that supported
+ * unmap on qcow2 backing files.
+ */
+bool
+guestfs___discard_possible (guestfs_h *g, struct drive *drv,
+                            bool qemu15, bool qemu16)
+{
+  if (!qemu15) {
+    error (g, _("discard cannot be enabled on this drive: "
+                "qemu < 1.5"));
+    return false;
+  }
+
+  /* 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) {
+    error (g, _("discard cannot be enabled on this drive: "
+                "the drive has a read-only overlay"));
+    return false;
+  }
+
+  /* Look at the source format. */
+  if (drv->src.format == NULL) {
+    /* We could autodetect the format, but we don't ... yet. XXX */
+    error (g, _("discard cannot be enabled on this drive: "
+                "you have to specify the format of the file"));
+    return false;
+  }
+  else if (STREQ (drv->src.format, "raw"))
+    /* OK */ ;
+  else if (STREQ (drv->src.format, "qcow2")) {
+    if (!qemu16) {
+      error (g, _("discard cannot be enabled on this drive: "
+                  "qemu < 1.6 cannot do discard on qcow2 files"));
+    return false;
+    }
+  }
+  else {
+    /* It's possible in future other formats will support discard, but
+     * currently (qemu 1.7) none of them do.
+     */
+    error (g, _("discard cannot be enabled on this drive: "
+                "qemu does not support discard for '%s' format
files"),
+           drv->src.format);
+    return false;
+  }
+
+  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:
+    error (g, _("discard cannot be enabled on this drive: "
+                "protocol '%s' does not support discard"),
+           guestfs___drive_protocol_to_string (drv->src.protocol));
+    return false;
+  }
+
+  return true;
+}
+
 static int
 shutdown_direct (guestfs_h *g, void *datav, int check_for_errors)
 {
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index d86e2d2..1c46b57 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -107,6 +107,7 @@ struct backend_libvirt_data {
   bool selinux_norelabel_disks;
   char name[DOMAIN_NAME_LEN];   /* random name */
   bool is_kvm;                  /* false = qemu, true = kvm (from
capabilities)*/
+  unsigned long qemu_version;   /* qemu version (from libvirt) */
 };
 
 /* Parameters passed to construct_libvirt_xml and subfunctions.  We
@@ -131,6 +132,7 @@ static xmlChar *construct_libvirt_xml (guestfs_h *g, const
struct backend_libvir
 static void debug_appliance_permissions (guestfs_h *g);
 static void debug_socket_permissions (guestfs_h *g);
 static void libvirt_error (guestfs_h *g, const char *fs, ...)
__attribute__((format (printf,2,3)));
+static void libvirt_debug (guestfs_h *g, const char *fs, ...)
__attribute__((format (printf,2,3)));
 static int is_custom_hv (guestfs_h *g);
 static int is_blk (const char *path);
 static void ignore_errors (void *ignore, virErrorPtr ignore2);
@@ -283,6 +285,19 @@ launch_libvirt (guestfs_h *g, void *datav, const char
*libvirt_uri)
    */
   virConnSetErrorFunc (conn, NULL, ignore_errors);
 
+  /* Get hypervisor (hopefully qemu) version. */
+  if (virConnectGetVersion (conn, &data->qemu_version) == 0) {
+    debug (g, "qemu version (reported by libvirt) = %lu
(%lu.%lu.%lu)",
+           data->qemu_version,
+           data->qemu_version / 1000000UL,
+           data->qemu_version / 1000UL % 1000UL,
+           data->qemu_version % 1000UL);
+  }
+  else {
+    libvirt_debug (g, "unable to read qemu version from libvirt");
+    data->qemu_version = 0;
+  }
+
   if (g->verbose)
     guestfs___print_timestamped_message (g, "get libvirt
capabilities");
 
@@ -789,7 +804,7 @@ static int construct_libvirt_xml_devices (guestfs_h *g,
const struct libvirt_xml
 static int construct_libvirt_xml_qemu_cmdline (guestfs_h *g, const struct
libvirt_xml_params *params, xmlTextWriterPtr xo);
 static int construct_libvirt_xml_disk (guestfs_h *g, const struct
backend_libvirt_data *data, xmlTextWriterPtr xo, struct drive *drv, size_t
drv_index);
 static int construct_libvirt_xml_disk_target (guestfs_h *g, xmlTextWriterPtr
xo, size_t drv_index);
-static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g,
xmlTextWriterPtr xo, const char *format, const char *cachemode);
+static int construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, const struct
backend_libvirt_data *data, struct drive *drv, xmlTextWriterPtr xo, const char
*format, const char *cachemode, enum discard discard);
 static int construct_libvirt_xml_disk_address (guestfs_h *g, xmlTextWriterPtr
xo, size_t drv_index);
 static int construct_libvirt_xml_disk_source_hosts (guestfs_h *g,
xmlTextWriterPtr xo, const struct drive_source *src);
 static int construct_libvirt_xml_disk_source_seclabel (guestfs_h *g, const
struct backend_libvirt_data *data, xmlTextWriterPtr xo);
@@ -1238,7 +1253,9 @@ construct_libvirt_xml_disk (guestfs_h *g,
       if (construct_libvirt_xml_disk_target (g, xo, drv_index) == -1)
         return -1;
 
-      if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2",
"unsafe")
+      if (construct_libvirt_xml_disk_driver_qemu (g, NULL, NULL,
+                                                  xo, "qcow2",
"unsafe",
+                                                  discard_disable)
           == -1)
         return -1;
     }
@@ -1375,8 +1392,9 @@ construct_libvirt_xml_disk (guestfs_h *g,
         return -1;
       }
 
-      if (construct_libvirt_xml_disk_driver_qemu (g, xo, format,
-                                                  drv->cachemode ? :
"writeback")
+      if (construct_libvirt_xml_disk_driver_qemu (g, data, drv, xo, format,
+                                                  drv->cachemode ? :
"writeback",
+                                                  drv->discard)
           == -1)
         return -1;
     }
@@ -1412,14 +1430,43 @@ construct_libvirt_xml_disk_target (guestfs_h *g,
xmlTextWriterPtr xo,
 }
 
 static int
-construct_libvirt_xml_disk_driver_qemu (guestfs_h *g, xmlTextWriterPtr xo,
+construct_libvirt_xml_disk_driver_qemu (guestfs_h *g,
+                                        const struct backend_libvirt_data
*data,
+                                        struct drive *drv,
+                                        xmlTextWriterPtr xo,
                                         const char *format,
-                                        const char *cachemode)
+                                        const char *cachemode,
+                                        enum discard discard)
 {
+  bool discard_unmap = false;
+
+  switch (discard) {
+  case discard_disable:
+    /* Since the default is always discard=ignore, don't specify it
+     * in the XML.
+     */
+    break;
+  case discard_enable:
+    if (!guestfs___discard_possible (g, drv,
+                                     data->qemu_version >= 1005000,
+                                     data->qemu_version >= 1006000))
+      return -1;
+    /*FALLTHROUGH*/
+  case discard_besteffort:
+    /* I believe from reading the code that this is always safe as
+     * long as qemu >= 1.5.
+     */
+    if (data->qemu_version >= 1005000)
+      discard_unmap = true;
+    break;
+  }
+
   start_element ("driver") {
     attribute ("name", "qemu");
     attribute ("type", format);
     attribute ("cache", cachemode);
+    if (discard_unmap)
+      attribute ("discard", "unmap");
   } end_element ();
 
   return 0;
@@ -1504,7 +1551,9 @@ construct_libvirt_xml_appliance (guestfs_h *g,
       attribute ("bus", "scsi");
     } end_element ();
 
-    if (construct_libvirt_xml_disk_driver_qemu (g, xo, "qcow2",
"unsafe") == -1)
+    if (construct_libvirt_xml_disk_driver_qemu (g, NULL, NULL, xo,
+                                                "qcow2",
"unsafe",
+                                                discard_disable) == -1)
       return -1;
 
     if (construct_libvirt_xml_disk_address (g, xo, params->appliance_index)
@@ -1664,6 +1713,39 @@ libvirt_error (guestfs_h *g, const char *fs, ...)
   free (msg);
 }
 
+/* Same as 'libvirt_error' but calls debug instead. */
+static void
+libvirt_debug (guestfs_h *g, const char *fs, ...)
+{
+  va_list args;
+  char *msg;
+  int len;
+  virErrorPtr err;
+
+  if (!g->verbose)
+    return;
+
+  va_start (args, fs);
+  len = vasprintf (&msg, fs, args);
+  va_end (args);
+
+  if (len < 0)
+    msg = safe_asprintf (g,
+                         _("%s: internal error forming error
message"),
+                         __func__);
+
+  /* In all recent libvirt, this retrieves the thread-local error. */
+  err = virGetLastError ();
+  if (err)
+    debug (g, "%s: %s [code=%d domain=%d]",
+           msg, err->message, err->code, err->domain);
+  else
+    debug (g, "%s", msg);
+
+  /* NB. 'err' must not be freed! */
+  free (msg);
+}
+
 #if HAVE_LIBSELINUX
 static void
 selinux_warning (guestfs_h *g, const char *func,
diff --git a/src/launch-uml.c b/src/launch-uml.c
index deda366..2a6ddaf 100644
--- a/src/launch-uml.c
+++ b/src/launch-uml.c
@@ -122,6 +122,12 @@ uml_supported (guestfs_h *g)
              _("uml backend does not support drives with 'label'
parameter"));
       return false;
     }
+    /* Note that discard == "besteffort" is fine. */
+    if (drv->discard == discard_enable) {
+      error (g,
+             _("uml backend does not support drives with 'discard'
parameter set to 'enable'"));
+      return false;
+    }
   }
 
   return true;
-- 
1.8.5.3

Test that fstrim actually works, end-to-end.
---
 Makefile.am                   |   1 +
 configure.ac                  |   1 +
 tests/discard/Makefile.am     |  26 ++++++++++
 tests/discard/test-discard.pl | 116 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+)
 create mode 100644 tests/discard/Makefile.am
 create mode 100755 tests/discard/test-discard.pl

diff --git a/Makefile.am b/Makefile.am
index 020628e..11b12c2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -46,6 +46,7 @@ SUBDIRS += tests/events
 SUBDIRS += tests/parallel
 SUBDIRS += tests/create
 SUBDIRS += tests/disks
+SUBDIRS += tests/discard
 SUBDIRS += tests/mountable
 SUBDIRS += tests/network
 SUBDIRS += tests/lvm
diff --git a/configure.ac b/configure.ac
index 35460e2..3785ea8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1673,6 +1673,7 @@ AC_CONFIG_FILES([Makefile
                  tests/charsets/Makefile
                  tests/create/Makefile
                  tests/data/Makefile
+                 tests/discard/Makefile
                  tests/disks/Makefile
                  tests/disks/test-qemu-drive-libvirt.xml
                  tests/disk-labels/Makefile
diff --git a/tests/discard/Makefile.am b/tests/discard/Makefile.am
new file mode 100644
index 0000000..f159287
--- /dev/null
+++ b/tests/discard/Makefile.am
@@ -0,0 +1,26 @@
+# libguestfs
+# Copyright (C) 2014 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+include $(top_srcdir)/subdir-rules.mk
+
+TESTS = \
+	test-discard.pl
+
+TESTS_ENVIRONMENT = $(top_builddir)/run --test
+
+EXTRA_DIST = \
+	$(TESTS)
diff --git a/tests/discard/test-discard.pl b/tests/discard/test-discard.pl
new file mode 100755
index 0000000..770049c
--- /dev/null
+++ b/tests/discard/test-discard.pl
@@ -0,0 +1,116 @@
+#!/usr/bin/perl
+# Copyright (C) 2014 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test that discard support works.
+
+use strict;
+use warnings;
+
+use Sys::Guestfs;
+
+# Since we read error messages, we want to ensure they are printed
+# in English, hence:
+$ENV{"LANG"} = "C";
+
+$| = 1;
+
+my $g = Sys::Guestfs->new ();
+
+# Discard is only supported when using qemu.
+if ($g->get_backend () ne "libvirt" &&
+    $g->get_backend () !~ /^libvirt:/ &&
+    $g->get_backend () ne "direct") {
+    print "$0: skipped test because discard is only supported when using
qemu\n";
+    exit 77;
+}
+
+# You can set this to "raw" or "qcow2".
+my $format = "raw";
+
+my $disk;
+my @args;
+if ($format eq "raw") {
+    $disk = "test-discard.img";
+    @args = ( preallocation => "sparse" );
+} elsif ($format eq "qcow2") {
+    $disk = "test-discard.qcow2";
+    @args = ( preallocation => "off", compat => "1.1"
);
+} else {
+    die "$0: invalid disk format: $format\n";
+}
+
+# Create a disk and add it with discard enabled.  This is allowed to
+# fail, eg because qemu is too old, but libguestfs must tell us that
+# it failed (since we're using 'enable', not 'besteffort').
+$g->disk_create ($disk, $format, 100*1024*1024, @args);
+END { unlink ($disk); };
+
+eval {
+    $g->add_drive ($disk, format => $format, readonly => 0, discard
=> "enable");
+    $g->launch ();
+};
+if ($@) {
+    if ($@ =~ /discard cannot be enabled on this drive/) {
+        # This is OK.  Libguestfs says it's not possible to enable
+        # discard on this drive (eg. because qemu is too old).  Print
+        # the reason and skip the test.
+        print "$0: skipped test: $@\n";
+        exit 77;
+    }
+    die # propagate the unexpected error
+}
+
+# Is fstrim available in the appliance?
+unless ($g->feature_available (["fstrim"])) {
+    print "$0: skipped test because fstrim is not available\n";
+    exit 77;
+}
+
+# At this point we've got a disk which claims to support discard.
+# Let's test that theory.
+
+my $orig_size = (stat ($disk))[12];
+print "original size:\t$orig_size (blocks)\n";
+#system "du -sh $disk";
+
+# Write a filesystem onto the disk and fill it with data.
+
+$g->mkfs ("ext4", "/dev/sda");
+$g->mount_options ("discard", "/dev/sda",
"/");
+$g->fill (33, 10000000, "/data");
+$g->sync ();
+
+my $full_size = (stat ($disk))[12];
+print "full size:\t$full_size (blocks)\n";
+#system "du -sh $disk";
+
+die "surprising result: full size <= original size"
+    if $full_size <= $orig_size;
+
+# Remove the file and then try to trim the filesystem.
+
+$g->rm ("/data");
+$g->fstrim ("/");
+$g->sync ();
+$g->close ();
+
+my $trimmed_size = (stat ($disk))[12];
+print "trimmed size:\t$trimmed_size (blocks)\n";
+#system "du -sh $disk";
+
+die "looks like the fstrim operation did not work"
+    if $trimmed_size >= $full_size;
-- 
1.8.5.3

Hi,

On Monday 10 March 2014 17:28:53 Richard W.M. Jones
wrote:
> index 1e84061..584c4de 100644
> --- a/src/launch-direct.c
> +++ b/src/launch-direct.c
> @@ -509,6 +509,29 @@ launch_direct (guestfs_h *g, void *datav, const char
*arg)
>      CLEANUP_FREE char *file = NULL, *escaped_file = NULL, *param = NULL;
> 
>      if (!drv->overlay) {
> +      const char *discard_mode = "";
> +      int major = data->qemu_version_major, minor =
data->qemu_version_minor;
> +
> +      switch (drv->discard) {
> +      case discard_disable:
> +        /* Since the default is always discard=ignore, don't specify
it
> +         * on the command line.  This also avoids unnecessary breakage
> +         * with qemu < 1.5 which didn't have the option at all.
> +         */
> +        break;
> +      case discard_enable:
> +        if (!guestfs___discard_possible (g, drv, major, minor))
There is an argument mismatch in this call to guestfs___discard_possible
(int's vs bool's).

What about turn guestfs___discard_possible to just get the qemu version,
and do the checks in it rather than asking to prefill the two
"is qemu >= 1.5" and "is qemu >= 1.6" bool's?

-- 
Pino Toscano

Hi,

On Monday 10 March 2014 17:28:52 Richard W.M. Jones
wrote:
> diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
> index fd6e6d2..d86e2d2 100644
> --- a/src/launch-libvirt.c
> +++ b/src/launch-libvirt.c
> @@ -106,6 +106,7 @@ struct backend_libvirt_data {
>    char *selinux_imagelabel;
>    bool selinux_norelabel_disks;
>    char name[DOMAIN_NAME_LEN];   /* random name */
> +  bool is_kvm;                  /* false = qemu, true = kvm (from
capabilities)*/ };
> 
>  /* Parameters passed to construct_libvirt_xml and subfunctions.  We
> @@ -122,12 +123,11 @@ struct libvirt_xml_params {
>    char guestfsd_path[UNIX_PATH_MAX]; /* paths to sockets */
>    char console_path[UNIX_PATH_MAX];
>    bool enable_svirt;            /* false if we decided to disable sVirt */
> -  bool is_kvm;                  /* false = qemu, true = kvm */
>    bool current_proc_is_root;    /* true = euid is root */
>  };
> 
> -static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct libvirt_xml_params *params);
> -static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
libvirt_xml_params *params);
> +static int parse_capabilities (guestfs_h *g, const char *capabilities_xml,
struct backend_libvirt_data *data);
> +static xmlChar *construct_libvirt_xml (guestfs_h *g, const struct
backend_libvirt_data *data, const struct libvirt_xml_params *params);
Considering the libvirt_xml_params struct has already a
'struct backend_libvirt_data *data' member, I guess these (and the
others) functions could just access params->data->is_kvm with no need
to pass also backend_libvirt_data around, no?
After all, construct_libvirt_xml_name does this already.

-- 
Pino Toscano