john cooper <john.cooper@xxxxxxxxxx> 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@xxxxxxxxxx>
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
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list