Hi, On Monday 10 March 2014 17:28:52 Richard W.M. Jones wrote: 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

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

On Mon, Mar 10, 2014 at 06:50:39PM +0100, Pino Toscano wrote: This is the result of two different attempts to write this function, and a lot of confusion :-( I'll fix it ... Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW