[libvirt] patch: allow disk cache mode to be specified in a domain's xml definition.
We have found certain application scenarios where overriding of the default qemu host cache mode provides a substantial improvement in guest performance. In particular, disabling host caching of the file/dev backing a guest drive. A summary of performance metrics may be found below. Attached is a simple patch which adds general support for a "cache=*" attribute to be passed to qemu from libvirt as part of a xml <driver> element. It is parsed in a domain's xml definition as well as being generated during output of the same. For example: <disk type='file' device='disk'> <source file='/guest_roots/fc8.root'/> <target dev='hda'/> <driver name='qemu' type='qwerty' cache='none'/> </disk> where both the existing "type=*" and added "cache=*" attributes are optional and independent. Note this is only intended to serve as an internal hook allowing override of qemu's default cache behavior. At the present it is not proposed as an end-user documented/visible feature. -john Mark Wagner wrote:
This data is provided is provided by the Red Hat Performance team. It is intended to support the request for adding the required functionality to libvirt to allow for setting a storage parameter to control the caching behavior of QEMU. This value would be passed on the QEMU command line.
Our testing of commercial databases in a larger, Enterprise configuration (MSA, 16 cores, etc) has shown that the default QEMU caching behavior is not always the best. This set of data compares the drop off in performance of both writethrough and writeback cache compared to the nocache option. The main metric is transactions per minute (tpm). The other "axis" is the number of simulated users (x1000)
% nocache TPM Results K Users WT WB 10 60% 64% 20 66% 71% 40 68% 72% 60 71% 75% 80 74% 79% 100 76% 83%
From the above set of data, it is clear that the default behavior of QEMU is the worst performer out of the three cache options for this type of use case. It is also clear that we at minimum, 25% of the possible TPM performance just due to the cache setting.
-- john.cooper@redhat.com
On Fri, Jan 16, 2009 at 03:46:01PM -0500, john cooper wrote:
We have found certain application scenarios where overriding of the default qemu host cache mode provides a substantial improvement in guest performance. In particular, disabling host caching of the file/dev backing a guest drive. A summary of performance metrics may be found below.
Attached is a simple patch which adds general support for a "cache=*" attribute to be passed to qemu from libvirt as part of a xml <driver> element. It is parsed in a domain's xml definition as well as being generated during output of the same. For example:
<disk type='file' device='disk'> <source file='/guest_roots/fc8.root'/> <target dev='hda'/> <driver name='qemu' type='qwerty' cache='none'/> </disk>
where both the existing "type=*" and added "cache=*" attributes are optional and independent.
That scheme looks good. FYI, I have a patch ready which supports the existing driver name & type attributes for QEMU/KVM, as per existing usage in Xen driver
domain_conf.c | 32 +++++++++++++++++++++++++------- domain_conf.h | 15 +++++++++++++++ qemu_conf.c | 32 ++++++++++++++++++++++++++++----
A couple of extra things needed - Addition to tests/qemuxml2argvtest.c to validate the XML to struct to QEMU ARGV conversion. - Addition to tests/qemuxml2xmltest.c to validate XML to struct to XML round-trip conversions. - Addition to the docs/libvirt.rng XML schema.
3 files changed, 68 insertions(+), 11 deletions(-) ================================================================= --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -80,6 +80,20 @@ enum virDomainDiskBus { VIR_DOMAIN_DISK_BUS_LAST };
+/* summary of all possible host open file cache modes + */ +typedef enum { + VIR_DOMAIN_DISK_CACHE_DEFAULT = 0, + VIR_DOMAIN_DISK_CACHE_DISABLE, + VIR_DOMAIN_DISK_CACHE_WRITEBACK, + VIR_DOMAIN_DISK_CACHE_WRITETHRU, + } host_cachemode;
Can you rename this typedef to virDomainDiskCache, and add a sentinal value VIR_DOMAIN_DISK_CACHE_LAST
+ +typedef struct { + char *tag; + host_cachemode idx; + } cache_map_t;
This 2nd struct is not required.
/* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -91,6 +105,7 @@ struct _virDomainDiskDef { char *dst; char *driverName; char *driverType; + host_cachemode cachemode;
Our code policy is to use 'int' when enums are used, since enums have no fixed size.
--- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -616,6 +616,26 @@ qemudNetworkIfaceConnect(virConnectPtr c return NULL; }
+/* map from internal cache mode to qemu cache arg text + */ +static cache_map_t qemu_cache_map[] = { + {"none", VIR_DOMAIN_DISK_CACHE_DISABLE}, + {"writeback", VIR_DOMAIN_DISK_CACHE_WRITEBACK}, + {"writethrough", VIR_DOMAIN_DISK_CACHE_WRITETHRU}, + {NULL}}; + +/* return qemu arg text * corresponding to idx if found, NULL otherwise + */ +static inline char *qemu_cache_mode(host_cachemode idx) +{ + int i; + + for (i = 0; qemu_cache_map[i].tag; ++i) + if (qemu_cache_map[i].idx == idx) + return (qemu_cache_map[i].tag); + return (NULL); +}
This static map & function are not required. Instead the int to char * conversion (and its reverse) are automatically generated through use of our built-in enum support macros In the domain_conf.h header file add VIR_ENUM_DECL(virDomainDiskCache) And then in the domain_conf.c file the impl is provided by VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST, "default", "none", "writeback", "writethrough"); This guarentees (with a compile check) that there is the correct number of constant strings, to match the enum values. You have two generated functions that can be used in the XML parsing/formatting calls int virDomainDiskCacheTypeFromString(const char *type); const char *virDomainDiskCacheTypeFromString(int type);
static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) @@ -946,6 +966,8 @@ int qemudBuildCommandLine(virConnectPtr virDomainDiskDefPtr disk = vm->def->disks[i]; int idx = virDiskNameToIndex(disk->dst); const char *bus = virDomainDiskQEMUBusTypeToString(disk->bus); + int nc; + char *cachemode;
if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { @@ -980,15 +1002,17 @@ int qemudBuildCommandLine(virConnectPtr break; }
- snprintf(opt, PATH_MAX, "file=%s,if=%s,%sindex=%d%s%s", + nc = snprintf(opt, PATH_MAX, "file=%s,if=%s,%sindex=%d%s", disk->src ? disk->src : "", bus, media ? media : "", idx, bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK - ? ",boot=on" : "", - disk->shared && ! disk->readonly - ? ",cache=off" : ""); + ? ",boot=on" : ""); + cachemode = qemu_cache_mode(disk->cachemode); + if (cachemode || disk->shared && !disk->readonly) + snprintf(opt + nc, PATH_MAX - nc, ",cache=%s", + cachemode ? cachemode : "off");
There have been two differnet syntaxes supported in QEMU for caching, so we need to detect this and switch between them. Originally there was just cache=on & cache=off (writethrough and no caching) Now it supports cache=writeback, cache=writethrough and cache=none|off So, if we detect the earlier syntax we need to raise an error if the user requested writeback (or translate it to 'cache=off' for safety sake).
ADD_ARG_LIT("-drive"); ADD_ARG_LIT(opt); ================================================================= --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -528,6 +528,16 @@ int virDomainDiskCompare(virDomainDiskDe
#ifndef PROXY + +/* map from xml cache tag to internal cache mode + */ +static cache_map_t cache_map[] = { + {"none", VIR_DOMAIN_DISK_CACHE_DISABLE}, + {"off", VIR_DOMAIN_DISK_CACHE_DISABLE}, + {"writeback", VIR_DOMAIN_DISK_CACHE_WRITEBACK}, + {"writethrough", VIR_DOMAIN_DISK_CACHE_WRITETHRU}, + {NULL}}; +
This chunk is not required, because the VIR_ENUM code takes care of this.
/* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -543,6 +553,8 @@ virDomainDiskDefParseXML(virConnectPtr c char *source = NULL; char *target = NULL; char *bus = NULL; + char *cachetag = NULL; + int i;
if (VIR_ALLOC(def) < 0) { virDomainReportError(conn, VIR_ERR_NO_MEMORY, NULL); @@ -592,6 +604,14 @@ virDomainDiskDefParseXML(virConnectPtr c (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); driverType = virXMLPropString(cur, "type"); + if (cachetag = virXMLPropString(cur, "cache")) { + for (i = 0; cache_map[i].tag; ++i) + if (!strcmp(cache_map[i].tag, cachetag)) { + def->cachemode = cache_map[i].idx; + break; + } + VIR_FREE(cachetag); + }
Indentation got a little mixed up here . The for() loop is can be replaced by a call to virDomainDiskCacheTypeFromString.
} else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -2620,14 +2640,12 @@ virDomainDiskDefFormat(virConnectPtr con type, device);
if (def->driverName) { + virBufferVSprintf(buf, " <driver name='%s'", def->driverName); if (def->driverType) - virBufferVSprintf(buf, - " <driver name='%s' type='%s'/>\n", - def->driverName, def->driverType); - else - virBufferVSprintf(buf, - " <driver name='%s'/>\n", - def->driverName); + virBufferVSprintf(buf, " type='%s'", def->driverType); + if (def->cachemode) + virBufferVSprintf(buf, " cache='%s'", cache_map[def->cachemode].tag);
The virDomainDiskCacheTypeToString() method should be used for this instead Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
A couple of extra things needed
- Addition to tests/qemuxml2argvtest.c to validate the XML to struct to QEMU ARGV conversion. - Addition to tests/qemuxml2xmltest.c to validate XML to struct to XML round-trip conversions. - Addition to the docs/libvirt.rng XML schema. Ok. Let's address the above when we're satisfied with
Daniel P. Berrange wrote: the state of the patch as we have at least one open issue (as below).
This static map & function are not required. Instead the int to char * conversion (and its reverse) are automatically generated through use of our built-in enum support macros
Yes you mentioned this in your earlier (off list) reply to me. I did produce a patch version using the VIR_ENUM_DECL/VIR_ENUM_IMPL macros but thought they may not be the most natural fit for the task since there is no way to multiply define a particular enum value (eg: cache "none" || "off"). Also a given hypervisor will likely support only a subset of the internally enumerated option space forcing dummy-out of unused cache modes. But that is speculative and I don't have a strong bias either way. So I've reverted to use of VIR_ENUM_DECL/VIR_ENUM_IMPL here.
There have been two differnet syntaxes supported in QEMU for caching, so we need to detect this and switch between them. Originally there was just
cache=on & cache=off (writethrough and no caching)
Now it supports
cache=writeback, cache=writethrough and cache=none|off
So, if we detect the earlier syntax we need to raise an error if the user requested writeback (or translate it to 'cache=off' for safety sake).
I was trying to address part of this as above. But in general I don't see how we can force an error in advance of launching qemu without some information beforehand of whether we're dealing with an old vs. new syntax qemu. One way to do so is to test launch "qemu --help" and let it tell us what it accepts. Alternatively we can just let qemu error exit in the case an old/new qemu version doesn't recognize the alternate syntax. Other than the above I believe I've incorporated all of your remaining suggestions. Attached is an updated patch. -john -- john.cooper@redhat.com
On Mon, Jan 19, 2009 at 10:06:47PM -0500, john cooper wrote:
There have been two differnet syntaxes supported in QEMU for caching, so we need to detect this and switch between them. Originally there was just
cache=on & cache=off (writethrough and no caching)
Now it supports
cache=writeback, cache=writethrough and cache=none|off
So, if we detect the earlier syntax we need to raise an error if the user requested writeback (or translate it to 'cache=off' for safety sake). I was trying to address part of this as above. But in general I don't see how we can force an error in advance of launching qemu without some information beforehand of whether we're dealing with an old vs. new syntax qemu. One way to do so is to test launch "qemu --help" and let it tell us what it accepts.
Bingo :-) We already have this problem for many QEMU arguments are do exactly that. If you loook at src/qemu_conf.c, you'll find a nice method called qemudExtractVersionInfo, which runs 'qemu -help' and checks for certain interesting command line arguments :-) So in src/qemu_conf.h just define a new enum value QEMUD_CMD_FLAG_NEW_CACHE and then check for something in the help output as per existing examples.
Alternatively we can just let qemu error exit in the case an old/new qemu version doesn't recognize the alternate syntax.
Other than the above I believe I've incorporated all of your remaining suggestions. Attached is an updated patch.
One note about the enums - rather than adding old style CACHEON CACHE_OFF options to the main enum in domain_conf, just create a second enum in the qemu_conf.c file for recording the mapping of virDomainDiskCache values to old style QEMU arguments values So in domain_conf.c you'd have VIR_ENUM_IMPL(virDomainDiskCacheNew, VIR_DOMAIN_DISK_CACHE_LAST, "default", "off", "writeback", "writethrough") While for the legacy qemu syntax can define VIR_ENUM_DECL(virDomainDiskCacheOld) VIR_ENUM_IMPL(virDomainDiskCacheOld, VIR_DOMAIN_DISK_CACHE_LAST, "default", "off", "off", "on") Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote:
If you loook at src/qemu_conf.c, you'll find a nice method called qemudExtractVersionInfo, which runs 'qemu -help' and checks for certain interesting command line arguments :-)
That problem does seem to be crying for some type of structured interface to avoid subtle breakage should someone modify the output of "--help". I'm sure I'm preaching to the choir here. So it now adapts for the cases of old syntax and "writethrough" as well as new syntax and "on" since qemu will otherwise balk at those cache flag / version combinations.
One note about the enums - rather than adding old style CACHEON CACHE_OFF options to the main enum in domain_conf, just create a second enum in the qemu_conf.c file for recording the mapping of virDomainDiskCache values to old style QEMU arguments values.. As we are adapting in both directions I left the single enum representing the entire option list to simplify things. Updated patch is attached.
-john -- john.cooper@redhat.com
john cooper <john.cooper@redhat.com> wrote:
Daniel P. Berrange wrote:
If you loook at src/qemu_conf.c, you'll find a nice method called qemudExtractVersionInfo, which runs 'qemu -help' and checks for certain interesting command line arguments :-) That problem does seem to be crying for some type of structured interface to avoid subtle breakage should someone modify the output of "--help". I'm sure I'm preaching to the choir here.
So it now adapts for the cases of old syntax and "writethrough" as well as new syntax and "on" since qemu will otherwise balk at those cache flag / version combinations.
One note about the enums - rather than adding old style CACHEON CACHE_OFF options to the main enum in domain_conf, just create a second enum in the qemu_conf.c file for recording the mapping of virDomainDiskCache values to old style QEMU arguments values.. As we are adapting in both directions I left the single enum representing the entire option list to simplify things. Updated patch is attached.
Hi John, I tried to apply that, but failed miserably, since all of the following was recently redone to use virBufferVSprintf rather than snprintf. And it's a good thing, because with the changes below, there was potential buffer overrun: nc could end up larger than PATH_MAX. Trying to apply, I hit a few other conflicts, but once I rewound far enough back, there was only one, and I dealt with that manually, then rebased and resolved the others. Once applied and up to date, I tried to build. That evoked a few compiler warnings due to e.g., assignment-in-if-expression context like below:
+ if (cachemode = disk->cachemode) {
I rewrote it like this: cachemode = disk->cachemode; if (cachemode) { There were 2 or 3 others that I fixed the same way. Also, I added this missing declaration in src/qemu_conf.c: VIR_ENUM_DECL(qemu_cache_map) Finally, I moved a couple variable declarations down (C99-style) to their points of first use. One remaining bit that I haven't done: add tests for this, exercising e.g., - cachemode - !cachemode && (disk->shared && !disk->readonly) - !cachemode && (!disk->shared || disk->readonly)
- snprintf(opt, PATH_MAX, "file=%s,if=%s,%sindex=%d%s%s", + nc = snprintf(opt, PATH_MAX, "file=%s,if=%s,%sindex=%d%s", disk->src ? disk->src : "", bus, media ? media : "", idx, bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK - ? ",boot=on" : "", - disk->shared && ! disk->readonly - ? ",cache=off" : ""); + ? ",boot=on" : ""); + + if (cachemode = disk->cachemode) { + if (qemudExtractVersionInfo(vm->def->emulator, NULL, &qf) < 0) + ; /* error reported */ + else if (!(qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE) + && cachemode == VIR_DOMAIN_DISK_CACHE_WRITETHRU) + cachemode = VIR_DOMAIN_DISK_CACHE_ON; + else if (qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE + && cachemode == VIR_DOMAIN_DISK_CACHE_ON) + cachemode = VIR_DOMAIN_DISK_CACHE_WRITETHRU; + } + if (cachemode || disk->shared && !disk->readonly) + snprintf(opt + nc, PATH_MAX - nc, ",cache=%s", + cachemode ? qemu_cache_mapTypeToString(cachemode) : "off");
Here's your rebased and adjusted patch:
From ce4f15853e119d6d976a5d29917f62f577e8ec9e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 29 Jan 2009 22:50:36 +0100 Subject: [PATCH] allow disk cache mode to be specified in a domain's xml definition.
Daniel P. Berrange wrote:
If you loook at src/qemu_conf.c, you'll find a nice method called qemudExtractVersionInfo, which runs 'qemu -help' and checks for certain interesting command line arguments :-)
That problem does seem to be crying for some type of structured interface to avoid subtle breakage should someone modify the output of "--help". I'm sure I'm preaching to the choir here. So it now adapts for the cases of old syntax and "writethrough" as well as new syntax and "on" since qemu will otherwise balk at those cache flag / version combinations.
One note about the enums - rather than adding old style CACHEON CACHE_OFF options to the main enum in domain_conf, just create a second enum in the qemu_conf.c file for recording the mapping of virDomainDiskCache values to old style QEMU arguments values.. As we are adapting in both directions I left the single enum representing the entire option list to simplify things. Updated patch is attached.
src/domain_conf.c | 34 ++++++++++++++++++++++++++-------- src/domain_conf.h | 16 ++++++++++++++++ src/qemu_conf.c | 41 ++++++++++++++++++++++++++++++++++++++--- src/qemu_conf.h | 3 ++- 4 files changed, 82 insertions(+), 12 deletions(-) diff --git a/src/domain_conf.c b/src/domain_conf.c index f696b6a..efd6981 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -551,6 +551,17 @@ int virDomainDiskCompare(virDomainDiskDefPtr a, #ifndef PROXY + +/* map from xml cache tag to internal cache mode + */ +VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST, + "", /* reserved -- mode not specified */ + "off", + "on", + "none", + "writeback", + "writethrough"); + /* Parse the XML definition for a disk * @param node XML nodeset to parse for disk definition */ @@ -567,6 +578,8 @@ virDomainDiskDefParseXML(virConnectPtr conn, char *source = NULL; char *target = NULL; char *bus = NULL; + char *cachetag = NULL; + int cm; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -616,6 +629,12 @@ virDomainDiskDefParseXML(virConnectPtr conn, (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); driverType = virXMLPropString(cur, "type"); + cachetag = virXMLPropString(cur, "cache"); + if (cachetag) { + if ((cm = virDomainDiskCacheTypeFromString(cachetag)) != -1) + def->cachemode = cm; + VIR_FREE(cachetag); + } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -2770,14 +2789,13 @@ virDomainDiskDefFormat(virConnectPtr conn, type, device); if (def->driverName) { + virBufferVSprintf(buf, " <driver name='%s'", def->driverName); if (def->driverType) - virBufferVSprintf(buf, - " <driver name='%s' type='%s'/>\n", - def->driverName, def->driverType); - else - virBufferVSprintf(buf, - " <driver name='%s'/>\n", - def->driverName); + virBufferVSprintf(buf, " type='%s'", def->driverType); + if (def->cachemode) + virBufferVSprintf(buf, " cache='%s'", + virDomainDiskCacheTypeToString(def->cachemode)); + virBufferVSprintf(buf, "/>\n"); } if (def->src) { diff --git a/src/domain_conf.h b/src/domain_conf.h index 09afd1f..c72c0dc 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -81,6 +81,21 @@ enum virDomainDiskBus { VIR_DOMAIN_DISK_BUS_LAST }; +/* summary of all possible host open file cache modes + */ +typedef enum { + VIR_DOMAIN_DISK_CACHE_UNSPECIFIED, /* reserved */ + VIR_DOMAIN_DISK_CACHE_OFF, + VIR_DOMAIN_DISK_CACHE_ON, + VIR_DOMAIN_DISK_CACHE_DISABLE, + VIR_DOMAIN_DISK_CACHE_WRITEBACK, + VIR_DOMAIN_DISK_CACHE_WRITETHRU, + + VIR_DOMAIN_DISK_CACHE_LAST + } virDomainDiskCache; + +VIR_ENUM_DECL(virDomainDiskCache) + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -92,6 +107,7 @@ struct _virDomainDiskDef { char *dst; char *driverName; char *driverType; + int cachemode; unsigned int readonly : 1; unsigned int shared : 1; int slotnum; /* pci slot number for unattach */ diff --git a/src/qemu_conf.c b/src/qemu_conf.c index 890434f..d65dc12 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1,7 +1,7 @@ /* * config.c: VM configuration management * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -399,7 +399,9 @@ int qemudExtractVersionInfo(const char *qemu, if (strstr(help, "-domid")) flags |= QEMUD_CMD_FLAG_DOMID; if (strstr(help, "-drive")) - flags |= QEMUD_CMD_FLAG_DRIVE; + flags |= QEMUD_CMD_FLAG_DRIVE | + (strstr(help, "cache=writethrough|writeback|none") ? + QEMUD_CMD_FLAG_DRIVE_CACHEMODE : 0); if (strstr(help, "boot=on")) flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; if (version >= 9000) @@ -591,6 +593,22 @@ qemudNetworkIfaceConnect(virConnectPtr conn, return NULL; } +VIR_ENUM_DECL(qemu_cache_map) + +/* map from internal cache mode to qemu cache arg text + * + * Note: currently this replicates virDomainDiskCache, but will need to + * error flag potential new entries in virDomainDiskCache which are + * not supported by qemu (raising exceptions as appropriate). + */ +VIR_ENUM_IMPL(qemu_cache_map, VIR_DOMAIN_DISK_CACHE_LAST, + "", /* reserved -- mode not specified */ + "off", /* deprecated; use "none" */ + "on", /* obsolete; use "writethrough" */ + "none", + "writeback", + "writethrough") + static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) @@ -1012,6 +1030,24 @@ int qemudBuildCommandLine(virConnectPtr conn, if (disk->driverType) virBufferVSprintf(&opt, ",fmt=%s", disk->driverType); + unsigned int qf; + int cachemode = disk->cachemode; + if (cachemode) { + if (qemudExtractVersionInfo(vm->def->emulator, NULL, &qf) < 0) + ; /* error reported */ + else if (!(qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE) + && cachemode == VIR_DOMAIN_DISK_CACHE_WRITETHRU) + cachemode = VIR_DOMAIN_DISK_CACHE_ON; + else if (qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE + && cachemode == VIR_DOMAIN_DISK_CACHE_ON) + cachemode = VIR_DOMAIN_DISK_CACHE_WRITETHRU; + } + if (cachemode || (disk->shared && !disk->readonly)) + virBufferVSprintf(&opt, ",cache=%s", + (cachemode + ? qemu_cache_mapTypeToString(cachemode) + : "off")); + if (virBufferError(&opt)) { virReportOOMError(conn); goto error; @@ -1577,4 +1613,3 @@ cleanup: VIR_FREE(xml); return ret; } - diff --git a/src/qemu_conf.h b/src/qemu_conf.h index 66bd61c..9bf4f31 100644 --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -1,7 +1,7 @@ /* * config.h: VM configuration management * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -53,6 +53,7 @@ enum qemud_cmd_flags { * since it had a design bug blocking the entire monitor console */ QEMUD_CMD_FLAG_MIGRATE_QEMU_TCP = (1 << 10), /* New migration syntax after merge to QEMU with TCP transport */ QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC = (1 << 11), /* New migration syntax after merge to QEMU with EXEC transport */ + QEMUD_CMD_FLAG_DRIVE_CACHEMODE = (1 << 12), /* Is the -cache option available */ }; /* Main driver state */ -- 1.6.1.2.418.gd79e6
Jim Meyering wrote:
Finally, I moved a couple variable declarations down (C99-style) to their points of first use.
I take it that the C++/C99 is the recommended style for all libvirt code? I generally haven't coded this way in the past (in fact I usually compile with -Wdeclaration-after-statement), and not seeing anything in HACKING about it, I continued to use the traditional style, but if this way is what everyone's converging on, I'll change to it. Dave
On Thu, Jan 29, 2009 at 10:48:36PM -0500, Dave Allan wrote:
Jim Meyering wrote:
Finally, I moved a couple variable declarations down (C99-style) to their points of first use.
I take it that the C++/C99 is the recommended style for all libvirt code? I generally haven't coded this way in the past (in fact I usually compile with -Wdeclaration-after-statement), and not seeing anything in HACKING about it, I continued to use the traditional style, but if this way is what everyone's converging on, I'll change to it.
My preference is for declarations immediately after the start of a block. eg, at start of a function, or immediately at start of a nested if / while. I don't like have declarations scattered elsewhere in the functions because it means you end up having to scan the entire function body to find where variables are declared, instead of just looking at the start of the code block you are in. The vast majority of libvirt code works this way, so I'd rather stick with the traditional style and cleanup the few places not in compliance. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Dave Allan <dallan@redhat.com> wrote:
Jim Meyering wrote:
Finally, I moved a couple variable declarations down (C99-style) to their points of first use.
I take it that the C++/C99 is the recommended style for all libvirt code? I generally haven't coded this way in the past (in fact I usually compile with -Wdeclaration-after-statement), and not seeing anything in HACKING about it, I continued to use the traditional style, but if this way is what everyone's converging on, I'll change to it.
Until a couple years ago, the de-facto standard in libvirt (like many projects) was to use C89-style all-declarations-at-opening-brace, but since no reasonable portability target lacks a C99 compiler, we are slowly modernizing with the addition of new code. If you would like a long list of reasons for using this aspect of C99, let me know. Last night I was peeved to have to deal with a pointless conflict because a declaration-addition evoked a conflict. Guess why? Because adding it at the beginning of the function conflicted with another change that had also appended to that "function-global" list, but for a different variable name. Ironically, both were for uses that were much farther down and very local, so putting the respective declarations near their uses would have made the patches and code smaller and more readable, in addition to avoiding my conflict. I propose mention in HACKING that it is permitted (encouraged, even) to use C99 decls-after-stmt, but that you won't get dinged (yet) for using the old style. IME, readability and maintainability are important enough to require minimum distance between decl and first use of each variable, but others may have different habits and resist the change. Let's see.
On Fri, Jan 30, 2009 at 12:07:31AM +0100, Jim Meyering wrote:
Here's your rebased and adjusted patch:
From ce4f15853e119d6d976a5d29917f62f577e8ec9e Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 29 Jan 2009 22:50:36 +0100 Subject: [PATCH] allow disk cache mode to be specified in a domain's xml definition.
Daniel P. Berrange wrote:
If you loook at src/qemu_conf.c, you'll find a nice method called qemudExtractVersionInfo, which runs 'qemu -help' and checks for certain interesting command line arguments :-)
That problem does seem to be crying for some type of structured interface to avoid subtle breakage should someone modify the output of "--help". I'm sure I'm preaching to the choir here.
So it now adapts for the cases of old syntax and "writethrough" as well as new syntax and "on" since qemu will otherwise balk at those cache flag / version combinations.
One note about the enums - rather than adding old style CACHEON CACHE_OFF options to the main enum in domain_conf, just create a second enum in the qemu_conf.c file for recording the mapping of virDomainDiskCache values to old style QEMU arguments values.. As we are adapting in both directions I left the single enum representing the entire option list to simplify things. Updated patch is attached.
src/domain_conf.c | 34 ++++++++++++++++++++++++++-------- src/domain_conf.h | 16 ++++++++++++++++ src/qemu_conf.c | 41 ++++++++++++++++++++++++++++++++++++++--- src/qemu_conf.h | 3 ++- 4 files changed, 82 insertions(+), 12 deletions(-)
diff --git a/src/domain_conf.c b/src/domain_conf.c index f696b6a..efd6981 100644 --- a/src/domain_conf.c +++ b/src/domain_conf.c @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -551,6 +551,17 @@ int virDomainDiskCompare(virDomainDiskDefPtr a,
#ifndef PROXY + +/* map from xml cache tag to internal cache mode + */ +VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST, + "", /* reserved -- mode not specified */ + "off", + "on", + "none", + "writeback", + "writethrough");
This is still wrong - the XML should only accept 'none', 'writeback', and 'writethrough', or 'default'. It shouldn't allow QEMU's 'on' or 'off' values - that's an internal impl detail
diff --git a/src/domain_conf.h b/src/domain_conf.h index 09afd1f..c72c0dc 100644 --- a/src/domain_conf.h +++ b/src/domain_conf.h @@ -81,6 +81,21 @@ enum virDomainDiskBus { VIR_DOMAIN_DISK_BUS_LAST };
+/* summary of all possible host open file cache modes + */ +typedef enum { + VIR_DOMAIN_DISK_CACHE_UNSPECIFIED, /* reserved */ + VIR_DOMAIN_DISK_CACHE_OFF, + VIR_DOMAIN_DISK_CACHE_ON, + VIR_DOMAIN_DISK_CACHE_DISABLE, + VIR_DOMAIN_DISK_CACHE_WRITEBACK, + VIR_DOMAIN_DISK_CACHE_WRITETHRU, + + VIR_DOMAIN_DISK_CACHE_LAST + } virDomainDiskCache;
Likewise ON/OFF should not be used here.
+VIR_ENUM_DECL(qemu_cache_map) + +/* map from internal cache mode to qemu cache arg text + * + * Note: currently this replicates virDomainDiskCache, but will need to + * error flag potential new entries in virDomainDiskCache which are + * not supported by qemu (raising exceptions as appropriate). + */ +VIR_ENUM_IMPL(qemu_cache_map, VIR_DOMAIN_DISK_CACHE_LAST, + "", /* reserved -- mode not specified */ + "off", /* deprecated; use "none" */ + "on", /* obsolete; use "writethrough" */ + "none", + "writeback", + "writethrough")
This needs to have two separate enums, one for old style, one for new style option naming
@@ -1012,6 +1030,24 @@ int qemudBuildCommandLine(virConnectPtr conn, if (disk->driverType) virBufferVSprintf(&opt, ",fmt=%s", disk->driverType);
+ unsigned int qf; + int cachemode = disk->cachemode; + if (cachemode) { + if (qemudExtractVersionInfo(vm->def->emulator, NULL, &qf) < 0) + ; /* error reported */
This will SEGV, because vm->def->emulator is not required to be present here. The command line flags are already extract & available further up in this method, so its redundant to extract them again.
+ else if (!(qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE) + && cachemode == VIR_DOMAIN_DISK_CACHE_WRITETHRU) + cachemode = VIR_DOMAIN_DISK_CACHE_ON; + else if (qf & QEMUD_CMD_FLAG_DRIVE_CACHEMODE + && cachemode == VIR_DOMAIN_DISK_CACHE_ON) + cachemode = VIR_DOMAIN_DISK_CACHE_WRITETHRU; + } + if (cachemode || (disk->shared && !disk->readonly)) + virBufferVSprintf(&opt, ",cache=%s", + (cachemode + ? qemu_cache_mapTypeToString(cachemode) + : "off"));
Further up in this code its already adding cache=off for shared disks. There's also quite a few code style issues, not following conventions of the surrounding code. Here's a patch which addresses all that, and more importantly adds test cases covering all the cache variants QEMU has. b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml | 30 +++++++ src/domain_conf.c | 36 +++++++-- src/domain_conf.h | 11 ++ src/qemu_conf.c | 38 ++++++++-- src/qemu_conf.h | 3 tests/qemuxml2argvtest.c | 10 ++ tests/qemuxml2xmltest.c | 3 20 files changed, 298 insertions(+), 14 deletions(-) Daniel diff -r 2ff2ff7734c2 src/domain_conf.c --- a/src/domain_conf.c Fri Jan 30 11:01:52 2009 +0000 +++ b/src/domain_conf.c Fri Jan 30 12:28:00 2009 +0000 @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -91,6 +91,12 @@ VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMA "usb", "uml") +VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST, + "default", + "none", + "writethrough", + "writeback"); + VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "mount", "block", @@ -568,6 +574,7 @@ virDomainDiskDefParseXML(virConnectPtr c char *source = NULL; char *target = NULL; char *bus = NULL; + char *cachetag = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -617,6 +624,7 @@ virDomainDiskDefParseXML(virConnectPtr c (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); driverType = virXMLPropString(cur, "type"); + cachetag = virXMLPropString(cur, "cache"); } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -713,6 +721,13 @@ virDomainDiskDefParseXML(virConnectPtr c goto error; } + if (cachetag && + (def->cachemode = virDomainDiskCacheTypeFromString(cachetag)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown disk cache mode '%s'"), cachetag); + goto error; + } + def->src = source; source = NULL; def->dst = target; @@ -730,6 +745,7 @@ cleanup: VIR_FREE(device); VIR_FREE(driverType); VIR_FREE(driverName); + VIR_FREE(cachetag); return def; @@ -2756,6 +2772,7 @@ virDomainDiskDefFormat(virConnectPtr con const char *type = virDomainDiskTypeToString(def->type); const char *device = virDomainDiskDeviceTypeToString(def->device); const char *bus = virDomainDiskBusTypeToString(def->bus); + const char *cachemode = virDomainDiskCacheTypeToString(def->cachemode); if (!type) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -2772,20 +2789,23 @@ virDomainDiskDefFormat(virConnectPtr con _("unexpected disk bus %d"), def->bus); return -1; } + if (!cachemode) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unexpected disk cache mode %d"), def->cachemode); + return -1; + } virBufferVSprintf(buf, " <disk type='%s' device='%s'>\n", type, device); if (def->driverName) { + virBufferVSprintf(buf, " <driver name='%s'", def->driverName); if (def->driverType) - virBufferVSprintf(buf, - " <driver name='%s' type='%s'/>\n", - def->driverName, def->driverType); - else - virBufferVSprintf(buf, - " <driver name='%s'/>\n", - def->driverName); + virBufferVSprintf(buf, " type='%s'", def->driverType); + if (def->cachemode) + virBufferVSprintf(buf, " cache='%s'", cachemode); + virBufferVSprintf(buf, "/>\n"); } if (def->src) { diff -r 2ff2ff7734c2 src/domain_conf.h --- a/src/domain_conf.h Fri Jan 30 11:01:52 2009 +0000 +++ b/src/domain_conf.h Fri Jan 30 12:28:00 2009 +0000 @@ -81,6 +81,15 @@ enum virDomainDiskBus { VIR_DOMAIN_DISK_BUS_LAST }; +enum virDomainDiskCache { + VIR_DOMAIN_DISK_CACHE_DEFAULT, + VIR_DOMAIN_DISK_CACHE_DISABLE, + VIR_DOMAIN_DISK_CACHE_WRITETHRU, + VIR_DOMAIN_DISK_CACHE_WRITEBACK, + + VIR_DOMAIN_DISK_CACHE_LAST +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -92,6 +101,7 @@ struct _virDomainDiskDef { char *dst; char *driverName; char *driverType; + int cachemode; unsigned int readonly : 1; unsigned int shared : 1; int slotnum; /* pci slot number for unattach */ @@ -615,6 +625,7 @@ VIR_ENUM_DECL(virDomainLifecycle) VIR_ENUM_DECL(virDomainDisk) VIR_ENUM_DECL(virDomainDiskDevice) VIR_ENUM_DECL(virDomainDiskBus) +VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainNet) VIR_ENUM_DECL(virDomainChr) diff -r 2ff2ff7734c2 src/qemu_conf.c --- a/src/qemu_conf.c Fri Jan 30 11:01:52 2009 +0000 +++ b/src/qemu_conf.c Fri Jan 30 12:28:00 2009 +0000 @@ -1,7 +1,7 @@ /* * config.c: VM configuration management * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -61,6 +61,22 @@ VIR_ENUM_IMPL(virDomainDiskQEMUBus, VIR_ "uml") +VIR_ENUM_DECL(qemuDiskCacheV1) +VIR_ENUM_DECL(qemuDiskCacheV2) + +VIR_ENUM_IMPL(qemuDiskCacheV1, VIR_DOMAIN_DISK_CACHE_LAST, + "default", + "off", + "off", /* writethrough not supported, so for safety, disable */ + "on"); /* Old 'on' was equivalent to 'writeback' */ + +VIR_ENUM_IMPL(qemuDiskCacheV2, VIR_DOMAIN_DISK_CACHE_LAST, + "default", + "none", + "writethrough", + "writeback"); + + #define qemudLog(level, msg...) fprintf(stderr, msg) int qemudLoadDriverConfig(struct qemud_driver *driver, @@ -398,8 +414,11 @@ int qemudExtractVersionInfo(const char * flags |= QEMUD_CMD_FLAG_UUID; if (strstr(help, "-domid")) flags |= QEMUD_CMD_FLAG_DOMID; - if (strstr(help, "-drive")) + if (strstr(help, "-drive")) { flags |= QEMUD_CMD_FLAG_DRIVE; + if (strstr(help, "cache=writethrough|writeback|none")) + flags |= QEMUD_CMD_FLAG_DRIVE_CACHE_V2; + } if (strstr(help, "boot=on")) flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; if (version >= 9000) @@ -591,6 +610,7 @@ qemudNetworkIfaceConnect(virConnectPtr c return NULL; } + static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) @@ -1015,11 +1035,20 @@ int qemudBuildCommandLine(virConnectPtr if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) virBufferAddLit(&opt, ",boot=on"); - if (disk->shared && !disk->readonly) - virBufferAddLit(&opt, ",cache=off"); if (disk->driverType) virBufferVSprintf(&opt, ",fmt=%s", disk->driverType); + if (disk->cachemode) { + const char *mode = + (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_CACHE_V2) ? + qemuDiskCacheV2TypeToString(disk->cachemode) : + qemuDiskCacheV1TypeToString(disk->cachemode); + + virBufferVSprintf(&opt, ",cache=%s", mode); + } else if (disk->shared && !disk->readonly) { + virBufferAddLit(&opt, ",cache=off"); + } + if (virBufferError(&opt)) { virReportOOMError(conn); goto error; @@ -1585,4 +1614,3 @@ cleanup: VIR_FREE(xml); return ret; } - diff -r 2ff2ff7734c2 src/qemu_conf.h --- a/src/qemu_conf.h Fri Jan 30 11:01:52 2009 +0000 +++ b/src/qemu_conf.h Fri Jan 30 12:28:00 2009 +0000 @@ -1,7 +1,7 @@ /* * config.h: VM configuration management * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -53,6 +53,7 @@ enum qemud_cmd_flags { * since it had a design bug blocking the entire monitor console */ QEMUD_CMD_FLAG_MIGRATE_QEMU_TCP = (1 << 10), /* New migration syntax after merge to QEMU with TCP transport */ QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC = (1 << 11), /* New migration syntax after merge to QEMU with EXEC transport */ + QEMUD_CMD_FLAG_DRIVE_CACHE_V2 = (1 << 12), /* Is the cache= flag wanting new v2 values */ }; /* Main driver state */ diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=on -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writeback'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writethrough'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=none -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=writeback -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writeback'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=writethrough -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writethrough'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml Fri Jan 30 12:28:00 2009 +0000 @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <shareable/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvtest.c --- a/tests/qemuxml2argvtest.c Fri Jan 30 11:01:52 2009 +0000 +++ b/tests/qemuxml2argvtest.c Fri Jan 30 12:28:00 2009 +0000 @@ -201,6 +201,16 @@ mymain(int argc, char **argv) QEMUD_CMD_FLAG_DRIVE_BOOT); DO_TEST("disk-drive-fmt-qcow", QEMUD_CMD_FLAG_DRIVE | QEMUD_CMD_FLAG_DRIVE_BOOT); + DO_TEST("disk-drive-shared", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v1-wt", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v1-wb", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v1-none", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v2-wt", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + DO_TEST("disk-drive-cache-v2-wb", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + DO_TEST("disk-drive-cache-v2-none", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_CACHE_V2); DO_TEST("disk-usb", 0); DO_TEST("graphics-vnc", 0); DO_TEST("graphics-sdl", 0); diff -r 2ff2ff7734c2 tests/qemuxml2xmltest.c --- a/tests/qemuxml2xmltest.c Fri Jan 30 11:01:52 2009 +0000 +++ b/tests/qemuxml2xmltest.c Fri Jan 30 12:28:00 2009 +0000 @@ -98,6 +98,9 @@ mymain(int argc, char **argv) DO_TEST("disk-xenvbd"); DO_TEST("disk-usb"); DO_TEST("disk-drive-fmt-qcow"); + DO_TEST("disk-drive-cache-v1-wt"); + DO_TEST("disk-drive-cache-v1-wb"); + DO_TEST("disk-drive-cache-v1-none"); DO_TEST("graphics-vnc"); DO_TEST("graphics-sdl"); DO_TEST("graphics-sdl-fullscreen"); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Further up in this code its already adding cache=off for shared disks.
Probably introduced by me in the merge.
There's also quite a few code style issues, not following conventions of the surrounding code.
Here's a patch which addresses all that, and more importantly adds test cases covering all the cache variants QEMU has.
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml | 30 +++++++
I looked through the incremental changes, and they look ok, but see that they do introduce new test failures: 4) qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml FAILED Relax-NG validity error : Extra element devices in interleave /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml:15: element devices: Relax-NG validity error : Element domain failed to validate content /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml fails to validate ... 6) qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml FAILED Relax-NG validity error : Extra element devices in interleave /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml:15: element devices: Relax-NG validity error : Element domain failed to validate content /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml fails to validate ... 14) qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml FAILED Relax-NG validity error : Extra element devices in interleave /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml:15: element devices: Relax-NG validity error : Element domain failed to validate content /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml fails to validate ... ^R 17) qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml FAILED Relax-NG validity error : Extra element devices in interleave /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml:15: element devices: Relax-NG validity error : Element domain failed to validate content /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml fails to validate ... 22) qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml FAILED Relax-NG validity error : Extra element devices in interleave /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml:15: element devices: Relax-NG validity error : Element domain failed to validate content /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml fails to validate ... 42) qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml FAILED Relax-NG validity error : Extra element devices in interleave /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml:15: element devices: Relax-NG validity error : Element domain failed to validate content /c/libvirt/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml fails to validate
On Fri, Jan 30, 2009 at 03:27:55PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Further up in this code its already adding cache=off for shared disks.
Probably introduced by me in the merge.
There's also quite a few code style issues, not following conventions of the surrounding code.
Here's a patch which addresses all that, and more importantly adds test cases covering all the cache variants QEMU has.
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml | 30 +++++++
I looked through the incremental changes, and they look ok, but see that they do introduce new test failures:
Oh of course - I'll add this new parameter to the RNG schema - nice to see this test doing its job & preventing us adding new XML without corresponding schema updates :-) Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Jan 30, 2009 at 02:41:18PM +0000, Daniel P. Berrange wrote:
On Fri, Jan 30, 2009 at 03:27:55PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Further up in this code its already adding cache=off for shared disks.
Probably introduced by me in the merge.
There's also quite a few code style issues, not following conventions of the surrounding code.
Here's a patch which addresses all that, and more importantly adds test cases covering all the cache variants QEMU has.
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml | 29 +++++++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args | 1 b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml | 30 +++++++
I looked through the incremental changes, and they look ok, but see that they do introduce new test failures:
Oh of course - I'll add this new parameter to the RNG schema - nice to see this test doing its job & preventing us adding new XML without corresponding schema updates :-)
Here's the patch including RNG schema update Daniel diff -r 2ff2ff7734c2 docs/schemas/domain.rng --- a/docs/schemas/domain.rng Fri Jan 30 11:01:52 2009 +0000 +++ b/docs/schemas/domain.rng Fri Jan 30 14:47:11 2009 +0000 @@ -426,16 +426,43 @@ --> <define name='driver'> <element name='driver'> - <attribute name='name'> + <choice> + <group> + <ref name='driverFormat'/> + <optional> + <ref name='driverCache'/> + </optional> + </group> + <group> + <optional> + <ref name='driverFormat'/> + </optional> + <ref name='driverCache'/> + </group> + </choice> + <empty/> + </element> + </define> + + <define name='driverFormat'> + <attribute name='name'> + <ref name='genericName'/> + </attribute> + <optional> + <attribute name='type'> <ref name='genericName'/> </attribute> - <optional> - <attribute name='type'> - <ref name='genericName'/> - </attribute> - </optional> - <empty/> - </element> + </optional> + </define> + + <define name='driverCache'> + <attribute name='cache'> + <choice> + <value>none</value> + <value>writeback</value> + <value>writethrough</value> + </choice> + </attribute> </define> <define name='filesystem'> diff -r 2ff2ff7734c2 src/domain_conf.c --- a/src/domain_conf.c Fri Jan 30 11:01:52 2009 +0000 +++ b/src/domain_conf.c Fri Jan 30 14:47:11 2009 +0000 @@ -1,7 +1,7 @@ /* * domain_conf.c: domain XML processing * - * Copyright (C) 2006-2008 Red Hat, Inc. + * Copyright (C) 2006-2009 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -91,6 +91,12 @@ VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMA "usb", "uml") +VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST, + "default", + "none", + "writethrough", + "writeback"); + VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "mount", "block", @@ -568,6 +574,7 @@ virDomainDiskDefParseXML(virConnectPtr c char *source = NULL; char *target = NULL; char *bus = NULL; + char *cachetag = NULL; if (VIR_ALLOC(def) < 0) { virReportOOMError(conn); @@ -617,6 +624,7 @@ virDomainDiskDefParseXML(virConnectPtr c (xmlStrEqual(cur->name, BAD_CAST "driver"))) { driverName = virXMLPropString(cur, "name"); driverType = virXMLPropString(cur, "type"); + cachetag = virXMLPropString(cur, "cache"); } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -713,6 +721,13 @@ virDomainDiskDefParseXML(virConnectPtr c goto error; } + if (cachetag && + (def->cachemode = virDomainDiskCacheTypeFromString(cachetag)) < 0) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unknown disk cache mode '%s'"), cachetag); + goto error; + } + def->src = source; source = NULL; def->dst = target; @@ -730,6 +745,7 @@ cleanup: VIR_FREE(device); VIR_FREE(driverType); VIR_FREE(driverName); + VIR_FREE(cachetag); return def; @@ -2756,6 +2772,7 @@ virDomainDiskDefFormat(virConnectPtr con const char *type = virDomainDiskTypeToString(def->type); const char *device = virDomainDiskDeviceTypeToString(def->device); const char *bus = virDomainDiskBusTypeToString(def->bus); + const char *cachemode = virDomainDiskCacheTypeToString(def->cachemode); if (!type) { virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, @@ -2772,20 +2789,23 @@ virDomainDiskDefFormat(virConnectPtr con _("unexpected disk bus %d"), def->bus); return -1; } + if (!cachemode) { + virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("unexpected disk cache mode %d"), def->cachemode); + return -1; + } virBufferVSprintf(buf, " <disk type='%s' device='%s'>\n", type, device); if (def->driverName) { + virBufferVSprintf(buf, " <driver name='%s'", def->driverName); if (def->driverType) - virBufferVSprintf(buf, - " <driver name='%s' type='%s'/>\n", - def->driverName, def->driverType); - else - virBufferVSprintf(buf, - " <driver name='%s'/>\n", - def->driverName); + virBufferVSprintf(buf, " type='%s'", def->driverType); + if (def->cachemode) + virBufferVSprintf(buf, " cache='%s'", cachemode); + virBufferVSprintf(buf, "/>\n"); } if (def->src) { diff -r 2ff2ff7734c2 src/domain_conf.h --- a/src/domain_conf.h Fri Jan 30 11:01:52 2009 +0000 +++ b/src/domain_conf.h Fri Jan 30 14:47:11 2009 +0000 @@ -81,6 +81,15 @@ enum virDomainDiskBus { VIR_DOMAIN_DISK_BUS_LAST }; +enum virDomainDiskCache { + VIR_DOMAIN_DISK_CACHE_DEFAULT, + VIR_DOMAIN_DISK_CACHE_DISABLE, + VIR_DOMAIN_DISK_CACHE_WRITETHRU, + VIR_DOMAIN_DISK_CACHE_WRITEBACK, + + VIR_DOMAIN_DISK_CACHE_LAST +}; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -92,6 +101,7 @@ struct _virDomainDiskDef { char *dst; char *driverName; char *driverType; + int cachemode; unsigned int readonly : 1; unsigned int shared : 1; int slotnum; /* pci slot number for unattach */ @@ -615,6 +625,7 @@ VIR_ENUM_DECL(virDomainLifecycle) VIR_ENUM_DECL(virDomainDisk) VIR_ENUM_DECL(virDomainDiskDevice) VIR_ENUM_DECL(virDomainDiskBus) +VIR_ENUM_DECL(virDomainDiskCache) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainNet) VIR_ENUM_DECL(virDomainChr) diff -r 2ff2ff7734c2 src/qemu_conf.c --- a/src/qemu_conf.c Fri Jan 30 11:01:52 2009 +0000 +++ b/src/qemu_conf.c Fri Jan 30 14:47:11 2009 +0000 @@ -1,7 +1,7 @@ /* * config.c: VM configuration management * - * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -61,6 +61,22 @@ VIR_ENUM_IMPL(virDomainDiskQEMUBus, VIR_ "uml") +VIR_ENUM_DECL(qemuDiskCacheV1) +VIR_ENUM_DECL(qemuDiskCacheV2) + +VIR_ENUM_IMPL(qemuDiskCacheV1, VIR_DOMAIN_DISK_CACHE_LAST, + "default", + "off", + "off", /* writethrough not supported, so for safety, disable */ + "on"); /* Old 'on' was equivalent to 'writeback' */ + +VIR_ENUM_IMPL(qemuDiskCacheV2, VIR_DOMAIN_DISK_CACHE_LAST, + "default", + "none", + "writethrough", + "writeback"); + + #define qemudLog(level, msg...) fprintf(stderr, msg) int qemudLoadDriverConfig(struct qemud_driver *driver, @@ -398,8 +414,11 @@ int qemudExtractVersionInfo(const char * flags |= QEMUD_CMD_FLAG_UUID; if (strstr(help, "-domid")) flags |= QEMUD_CMD_FLAG_DOMID; - if (strstr(help, "-drive")) + if (strstr(help, "-drive")) { flags |= QEMUD_CMD_FLAG_DRIVE; + if (strstr(help, "cache=writethrough|writeback|none")) + flags |= QEMUD_CMD_FLAG_DRIVE_CACHE_V2; + } if (strstr(help, "boot=on")) flags |= QEMUD_CMD_FLAG_DRIVE_BOOT; if (version >= 9000) @@ -591,6 +610,7 @@ qemudNetworkIfaceConnect(virConnectPtr c return NULL; } + static int qemudBuildCommandLineChrDevStr(virDomainChrDefPtr dev, char *buf, int buflen) @@ -1015,11 +1035,20 @@ int qemudBuildCommandLine(virConnectPtr if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) virBufferAddLit(&opt, ",boot=on"); - if (disk->shared && !disk->readonly) - virBufferAddLit(&opt, ",cache=off"); if (disk->driverType) virBufferVSprintf(&opt, ",fmt=%s", disk->driverType); + if (disk->cachemode) { + const char *mode = + (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_CACHE_V2) ? + qemuDiskCacheV2TypeToString(disk->cachemode) : + qemuDiskCacheV1TypeToString(disk->cachemode); + + virBufferVSprintf(&opt, ",cache=%s", mode); + } else if (disk->shared && !disk->readonly) { + virBufferAddLit(&opt, ",cache=off"); + } + if (virBufferError(&opt)) { virReportOOMError(conn); goto error; @@ -1585,4 +1614,3 @@ cleanup: VIR_FREE(xml); return ret; } - diff -r 2ff2ff7734c2 src/qemu_conf.h --- a/src/qemu_conf.h Fri Jan 30 11:01:52 2009 +0000 +++ b/src/qemu_conf.h Fri Jan 30 14:47:11 2009 +0000 @@ -1,7 +1,7 @@ /* * config.h: VM configuration management * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2009 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -53,6 +53,7 @@ enum qemud_cmd_flags { * since it had a design bug blocking the entire monitor console */ QEMUD_CMD_FLAG_MIGRATE_QEMU_TCP = (1 << 10), /* New migration syntax after merge to QEMU with TCP transport */ QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC = (1 << 11), /* New migration syntax after merge to QEMU with EXEC transport */ + QEMUD_CMD_FLAG_DRIVE_CACHE_V2 = (1 << 12), /* Is the cache= flag wanting new v2 values */ }; /* Main driver state */ diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-none.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=on -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wb.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writeback'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v1-wt.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writethrough'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=none -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-none.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=writeback -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wb.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writeback'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=writethrough -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-cache-v2-wt.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='writethrough'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.args Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,index=0,fmt=qcow2,cache=off -drive file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,index=2,fmt=raw -net none -serial none -parallel none -usb diff -r 2ff2ff7734c2 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-shared.xml Fri Jan 30 14:47:11 2009 +0000 @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <shareable/> + </disk> + <disk type='block' device='cdrom'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + </devices> +</domain> diff -r 2ff2ff7734c2 tests/qemuxml2argvtest.c --- a/tests/qemuxml2argvtest.c Fri Jan 30 11:01:52 2009 +0000 +++ b/tests/qemuxml2argvtest.c Fri Jan 30 14:47:11 2009 +0000 @@ -201,6 +201,16 @@ mymain(int argc, char **argv) QEMUD_CMD_FLAG_DRIVE_BOOT); DO_TEST("disk-drive-fmt-qcow", QEMUD_CMD_FLAG_DRIVE | QEMUD_CMD_FLAG_DRIVE_BOOT); + DO_TEST("disk-drive-shared", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v1-wt", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v1-wb", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v1-none", QEMUD_CMD_FLAG_DRIVE); + DO_TEST("disk-drive-cache-v2-wt", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + DO_TEST("disk-drive-cache-v2-wb", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_CACHE_V2); + DO_TEST("disk-drive-cache-v2-none", QEMUD_CMD_FLAG_DRIVE | + QEMUD_CMD_FLAG_DRIVE_CACHE_V2); DO_TEST("disk-usb", 0); DO_TEST("graphics-vnc", 0); DO_TEST("graphics-sdl", 0); diff -r 2ff2ff7734c2 tests/qemuxml2xmltest.c --- a/tests/qemuxml2xmltest.c Fri Jan 30 11:01:52 2009 +0000 +++ b/tests/qemuxml2xmltest.c Fri Jan 30 14:47:11 2009 +0000 @@ -98,6 +98,9 @@ mymain(int argc, char **argv) DO_TEST("disk-xenvbd"); DO_TEST("disk-usb"); DO_TEST("disk-drive-fmt-qcow"); + DO_TEST("disk-drive-cache-v1-wt"); + DO_TEST("disk-drive-cache-v1-wb"); + DO_TEST("disk-drive-cache-v1-none"); DO_TEST("graphics-vnc"); DO_TEST("graphics-sdl"); DO_TEST("graphics-sdl-fullscreen"); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Here's the patch including RNG schema update
Daniel
diff -r 2ff2ff7734c2 docs/schemas/domain.rng --- a/docs/schemas/domain.rng Fri Jan 30 11:01:52 2009 +0000 +++ b/docs/schemas/domain.rng Fri Jan 30 14:47:11 2009 +0000 @@ -426,16 +426,43 @@ --> <define name='driver'> <element name='driver'> - <attribute name='name'> + <choice> + <group> + <ref name='driverFormat'/> + <optional> + <ref name='driverCache'/> + </optional> + </group> + <group> + <optional> + <ref name='driverFormat'/> + </optional> + <ref name='driverCache'/> + </group> + </choice> + <empty/> + </element> + </define> + + <define name='driverFormat'> + <attribute name='name'> + <ref name='genericName'/> + </attribute> + <optional> + <attribute name='type'> <ref name='genericName'/> </attribute> - <optional> - <attribute name='type'> - <ref name='genericName'/> - </attribute> - </optional> - <empty/> - </element> + </optional> + </define> + + <define name='driverCache'> + <attribute name='cache'> + <choice> + <value>none</value> + <value>writeback</value> + <value>writethrough</value> + </choice> + </attribute> </define>
<define name='filesystem'>
Yep. ACK.
Jim Meyering wrote:
Hi John,
I tried to apply that, but failed miserably, since all of the following was recently redone to use virBufferVSprintf rather than snprintf.
Yea I suspected the code was likely seeing some motion. Thanks for bringing it forward.
And it's a good thing, because with the changes below, there was potential buffer overrun: nc could end up larger than PATH_MAX. You are correct. I mistook the return value of snprintf() in the case of truncation.
One remaining bit that I haven't done: add tests for this, exercising e.g., - cachemode - !cachemode && (disk->shared && !disk->readonly) - !cachemode && (!disk->shared || disk->readonly)
That logic should be correct as it exists in the patch, namely we'll generate a "cache=*" flag in the case cachemode was specified explicitly in the xml definition or for the preexisting case of (shared && !readonly). Here if both happen to be true the explicit xml cache tag takes precedence. But with the existing code we need to remove the clause of: @@ -1007,18 +1025,34 @@ int qemudBuildCommandLine(virConnectPtr if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) virBufferAddLit(&opt, ",boot=on"); - if (disk->shared && !disk->readonly) - virBufferAddLit(&opt, ",cache=off"); if (disk->driverType) virBufferVSprintf(&opt, ",fmt=%s", disk->driverType); As this results in generation of a "cache=*" twice on the qemu cmdline, and possibly of different dispositions. With this change I think it is good to go. The attached patch incorporates the above modification. -john -- john.cooper@redhat.com
On Fri, Jan 30, 2009 at 10:10:34AM -0500, john cooper wrote:
With this change I think it is good to go. The attached patch incorporates the above modification.
NACK, please see my proposed patch elsewhere in this thread. daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (4)
-
Daniel P. Berrange -
Dave Allan -
Jim Meyering -
john cooper