Re: [libvirt] patch: allow disk cache mode to be specified in a domain's xml definition.

john cooper <john.cooper(a)redhat.com&gt; 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. 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: 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 (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: Jim Meyering <meyering(a)redhat.com&gt; 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: 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. 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