2:46 p.m.
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(a)redhat.com
6:58 a.m.
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 :|
9:06 p.m.
Daniel P. Berrange wrote:
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
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(a)redhat.com
4:13 a.m.
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 :|
3:42 p.m.
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(a)redhat.com
5:07 p.m.
john cooper <john.cooper(a)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.
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:
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. + 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(a)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
9:48 p.m.
New subject: style question (was Re: [libvirt] patch: allow disk cache mode to be specified in a domain's xml definition.)
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
4:02 a.m.
New subject: style question (was Re: [libvirt] patch: allow disk cache mode to be specified in a domain's xml definition.)
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 :|
4:06 a.m.
New subject: [libvirt] Re: style question
Dave Allan <dallan(a)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.
6:30 a.m.
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(a)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 :|
8:27 a.m.
"Daniel P. Berrange" <berrange(a)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
8:41 a.m.
On Fri, Jan 30, 2009 at 03:27:55PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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 :|
8:47 a.m.
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(a)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 :|
8:57 a.m.
"Daniel P. Berrange" <berrange(a)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.
9:10 a.m.
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(a)redhat.com
9:46 a.m.
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 :|
5777
days inactive
5791
days old
15 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Dave Allan -
Jim Meyering -
john cooper