[libvirt] [PATCHv3 1/3] save: support qemu modifying xml on domain save/restore
With this, it is possible to update the path to a disk backing image on either the save or restore action, without having to binary edit the XML embedded in the state file. This also modifies virDomainSave to output a smaller xml (only the inactive xml, which is all the more virDomainRestore parses), while still guaranteeing padding for most typical abi-compatible xml replacements, necessary so that the next patch for virDomainSaveImageDefineXML will not cause unnecessary modifications to the save image file. * src/qemu/qemu_driver.c (qemuDomainSaveInternal): Add parameter, only use inactive state, and guarantee padding. (qemuDomainSaveImageOpen): Add parameter. (qemuDomainSaveFlags, qemuDomainManagedSave) (qemuDomainRestoreFlags, qemuDomainObjRestore): Update callers. --- v3: change virDomainSave to always output minimal information, but with fixed padding added, so that save file modification will always be more likely to succeed, and not generate quite as many xml differences on round trips. src/qemu/qemu_driver.c | 109 +++++++++++++++++++++++++++++------------------ 1 files changed, 67 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1401967..2b1df6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2193,7 +2193,7 @@ qemuCompressProgramName(int compress) static int qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainObjPtr vm, const char *path, - int compressed, bool bypass_cache) + int compressed, bool bypass_cache, const char *xmlin) { char *xml = NULL; struct qemud_save_header header; @@ -2204,7 +2204,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, qemuDomainObjPrivatePtr priv; struct stat sb; bool is_reg = false; + size_t len; unsigned long long offset; + unsigned long long pad; int fd = -1; uid_t uid = getuid(); gid_t gid = getgid(); @@ -2239,15 +2241,54 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } } - /* Get XML for the domain */ - xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE); + /* Get XML for the domain. Restore needs only the inactive xml, + * including secure. We should get the same result whether xmlin + * is NULL or whether it was the live xml of the domain moments + * before. */ + if (xmlin) { + virDomainDefPtr def = NULL; + + if (!(def = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) { + goto endjob; + } + if (!virDomainDefCheckABIStability(vm->def, def)) { + virDomainDefFree(def); + goto endjob; + } + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE); + } else { + xml = virDomainDefFormat(vm->def, (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); + } if (!xml) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to get domain xml")); goto endjob; } - header.xml_len = strlen(xml) + 1; + len = strlen(xml) + 1; + offset = sizeof(header) + len; + + /* Due to way we append QEMU state on our header with dd, + * we need to ensure there's a 512 byte boundary. Unfortunately + * we don't have an explicit offset in the header, so we fake + * it by padding the XML string with NUL bytes. Additionally, + * we want to ensure that virDomainSaveImageDefineXML can supply + * slightly larger XML, so we add a miminum padding prior to + * rounding out to page boundaries. + */ + pad = 1024; + pad += (QEMU_MONITOR_MIGRATE_TO_FILE_BS - + ((offset + pad) % QEMU_MONITOR_MIGRATE_TO_FILE_BS)); + if (VIR_EXPAND_N(xml, len, pad) < 0) { + virReportOOMError(); + goto endjob; + } + offset += pad; + header.xml_len = len; + /* Obtain the file handle. */ /* path might be a pre-existing block dev, in which case * we need to skip the create step, and also avoid unlink * in the failure case */ @@ -2268,29 +2309,6 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } } - offset = sizeof(header) + header.xml_len; - - /* Due to way we append QEMU state on our header with dd, - * we need to ensure there's a 512 byte boundary. Unfortunately - * we don't have an explicit offset in the header, so we fake - * it by padding the XML string with NULLs. - */ - if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) { - unsigned long long pad = - QEMU_MONITOR_MIGRATE_TO_FILE_BS - - (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS); - - if (VIR_REALLOC_N(xml, header.xml_len + pad) < 0) { - virReportOOMError(); - goto endjob; - } - memset(xml + header.xml_len, 0, pad); - offset += pad; - header.xml_len += pad; - } - - /* Obtain the file handle. */ - /* First try creating the file as root */ if (bypass_cache) { directFlag = virFileDirectFdFlag(); @@ -2461,11 +2479,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, virDomainObjPtr vm = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); - if (dxml) { - qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("xml modification unsupported")); - return -1; - } qemuDriverLock(driver); @@ -2503,7 +2516,8 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, } ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + dxml); vm = NULL; cleanup: @@ -2567,7 +2581,8 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) compressed = QEMUD_SAVE_FORMAT_RAW; ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, - (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); + (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + NULL); vm = NULL; cleanup: @@ -3696,7 +3711,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, - bool bypass_cache, virFileDirectFdPtr *directFd) + bool bypass_cache, virFileDirectFdPtr *directFd, + const char *xmlin) { int fd; struct qemud_save_header header; @@ -3780,6 +3796,20 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, QEMU_EXPECTED_VIRT_TYPES, VIR_DOMAIN_XML_INACTIVE))) goto error; + if (xmlin) { + virDomainDefPtr def2 = NULL; + + if (!(def2 = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto error; + if (!virDomainDefCheckABIStability(def, def2)) { + virDomainDefFree(def2); + goto error; + } + virDomainDefFree(def); + def = def2; + } VIR_FREE(xml); @@ -3914,17 +3944,12 @@ qemuDomainRestoreFlags(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); - if (dxml) { - qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("xml modification unsupported")); - return -1; - } qemuDriverLock(driver); fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd); + &directFd, dxml); if (fd < 0) goto cleanup; @@ -3984,7 +4009,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd); + bypass_cache, &directFd, NULL); if (fd < 0) goto cleanup; -- 1.7.4.4
The goal here is that save-image-dumpxml fed back to save image-define without changing the save file; anywhere that this is not the case is probably a bug in domain_conf.c. * src/qemu/qemu_driver.c (qemuDomainSaveImageGetXMLDesc) (qemuDomainSaveImageDefineXML): New functions. (qemuDomainSaveImageOpen): Add parameter. (qemuDomainRestoreFlags, qemuDomainObjRestore): Adjust clients. --- v3: short cut exit with special value if no edits need to be made src/qemu/qemu_driver.c | 120 ++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 112 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b1df6c..f1a4e8a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3706,29 +3706,32 @@ cleanup: return ret; } -static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) +/* Return -1 on failure, -2 if edit was specified but xmlin does not + * represent any changes, and opened fd on all other success. */ +static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, bool bypass_cache, virFileDirectFdPtr *directFd, - const char *xmlin) + const char *xmlin, bool edit) { int fd; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; - int directFlag = 0; + int oflags = edit ? O_RDWR : O_RDONLY; if (bypass_cache) { - directFlag = virFileDirectFdFlag(); + int directFlag = virFileDirectFdFlag(); if (directFlag < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("bypass cache unsupported by this system")); goto error; } + oflags |= directFlag; } - if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + if ((fd = virFileOpenAs(path, oflags, 0, getuid(), getgid(), 0)) < 0) { if ((fd != -EACCES && fd != -EPERM) || driver->user == getuid()) { @@ -3739,7 +3742,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, /* Opening as root failed, but qemu runs as a different user * that might have better luck. */ - if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + if ((fd = virFileOpenAs(path, oflags, 0, driver->user, driver->group, VIR_FILE_OPEN_AS_UID)) < 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3791,6 +3794,15 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, goto error; } + if (edit && STREQ(xml, xmlin)) { + VIR_FREE(xml); + if (VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("cannot close file: %s"), path); + goto error; + } + return -2; + } + /* Create a domain from this XML */ if (!(def = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, @@ -3949,7 +3961,7 @@ qemuDomainRestoreFlags(virConnectPtr conn, fd = qemuDomainSaveImageOpen(driver, path, &def, &header, (flags & VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, - &directFd, dxml); + &directFd, dxml, false); if (fd < 0) goto cleanup; @@ -3995,6 +4007,96 @@ qemuDomainRestore(virConnectPtr conn, return qemuDomainRestoreFlags(conn, path, NULL, 0); } +static char * +qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, + unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + char *ret = NULL; + virDomainDefPtr def = NULL; + int fd = -1; + struct qemud_save_header header; + + /* We only take subset of virDomainDefFormat flags. */ + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + + qemuDriverLock(driver); + + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, + NULL, false); + + if (fd < 0) + goto cleanup; + + ret = qemuDomainDefFormatXML(driver, def, flags); + +cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, + const char *dxml, unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + int ret = -1; + virDomainDefPtr def = NULL; + int fd = -1; + struct qemud_save_header header; + char *xml = NULL; + size_t len; + + virCheckFlags(0, -1); + + qemuDriverLock(driver); + + fd = qemuDomainSaveImageOpen(driver, path, &def, &header, false, NULL, + dxml, true); + + if (fd < 0) { + /* Check for special case of no change needed. */ + if (fd == -2) + ret = 0; + goto cleanup; + } + + xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_SECURE); + if (!xml) + goto cleanup; + len = strlen(xml) + 1; + + if (len > header.xml_len) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("new xml too large to fit in file")); + } + if (VIR_EXPAND_N(xml, len, header.xml_len - len) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (lseek(fd, sizeof(header), SEEK_SET) != sizeof(header)) { + virReportSystemError(errno, _("cannot seek in '%s'"), path); + goto cleanup; + } + if (safewrite(fd, xml, len) != len || + VIR_CLOSE(fd) < 0) { + virReportSystemError(errno, _("failed to write xml to '%s'"), path); + goto cleanup; + } + + ret = 0; + +cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + VIR_FREE(xml); + qemuDriverUnlock(driver); + return ret; +} + static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, @@ -4009,7 +4111,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL; fd = qemuDomainSaveImageOpen(driver, path, &def, &header, - bypass_cache, &directFd, NULL); + bypass_cache, &directFd, NULL, false); if (fd < 0) goto cleanup; @@ -9070,6 +9172,8 @@ static virDriver qemuDriver = { .domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */ .domainRestore = qemuDomainRestore, /* 0.2.0 */ .domainRestoreFlags = qemuDomainRestoreFlags, /* 0.9.4 */ + .domainSaveImageGetXMLDesc = qemuDomainSaveImageGetXMLDesc, /* 0.9.4 */ + .domainSaveImageDefineXML = qemuDomainSaveImageDefineXML, /* 0.9.4 */ .domainCoreDump = qemudDomainCoreDump, /* 0.7.0 */ .domainScreenshot = qemuDomainScreenshot, /* 0.9.2 */ .domainSetVcpus = qemuDomainSetVcpus, /* 0.4.4 */ -- 1.7.4.4
On 07/22/2011 12:21 AM, Eric Blake wrote:
The goal here is that save-image-dumpxml fed back to save image-define without changing the save file; anywhere that this is not the case is probably a bug in domain_conf.c.
* src/qemu/qemu_driver.c (qemuDomainSaveImageGetXMLDesc) (qemuDomainSaveImageDefineXML): New functions. (qemuDomainSaveImageOpen): Add parameter. (qemuDomainRestoreFlags, qemuDomainObjRestore): Adjust clients. ---
v3: short cut exit with special value if no edits need to be made
src/qemu/qemu_driver.c | 120 ++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 112 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b1df6c..f1a4e8a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3706,29 +3706,32 @@ cleanup: return ret; }
-static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(6) +/* Return -1 on failure, -2 if edit was specified but xmlin does not + * represent any changes, and opened fd on all other success. */ +static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, bool bypass_cache, virFileDirectFdPtr *directFd, - const char *xmlin) + const char *xmlin, bool edit) { int fd; struct qemud_save_header header; char *xml = NULL; virDomainDefPtr def = NULL; - int directFlag = 0; + int oflags = edit ? O_RDWR : O_RDONLY;
if (bypass_cache) { - directFlag = virFileDirectFdFlag(); + int directFlag = virFileDirectFdFlag(); if (directFlag< 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("bypass cache unsupported by this system")); goto error; } + oflags |= directFlag; } - if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + if ((fd = virFileOpenAs(path, oflags, 0, getuid(), getgid(), 0))< 0) { if ((fd != -EACCES&& fd != -EPERM) || driver->user == getuid()) { @@ -3739,7 +3742,7 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver,
/* Opening as root failed, but qemu runs as a different user * that might have better luck. */ - if ((fd = virFileOpenAs(path, O_RDONLY | directFlag, 0, + if ((fd = virFileOpenAs(path, oflags, 0, driver->user, driver->group, VIR_FILE_OPEN_AS_UID))< 0) { qemuReportError(VIR_ERR_OPERATION_FAILED, @@ -3791,6 +3794,15 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, goto error; }
+ if (edit&& STREQ(xml, xmlin)) { + VIR_FREE(xml); + if (VIR_CLOSE(fd)< 0) { + virReportSystemError(errno, _("cannot close file: %s"), path); + goto error; + } + return -2; + }
So "unchanged" is indicated with a < 0 return code, but that only happens when edit == true, and only one of the callers does that (and properly handles the special case).
+ /* Create a domain from this XML */ if (!(def = virDomainDefParseString(driver->caps, xml, QEMU_EXPECTED_VIRT_TYPES, @@ -3949,7 +3961,7 @@ qemuDomainRestoreFlags(virConnectPtr conn,
fd = qemuDomainSaveImageOpen(driver, path,&def,&header, (flags& VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, -&directFd, dxml); +&directFd, dxml, false); if (fd< 0) goto cleanup;
@@ -3995,6 +4007,96 @@ qemuDomainRestore(virConnectPtr conn, return qemuDomainRestoreFlags(conn, path, NULL, 0); }
+static char * +qemuDomainSaveImageGetXMLDesc(virConnectPtr conn, const char *path, + unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + char *ret = NULL; + virDomainDefPtr def = NULL; + int fd = -1; + struct qemud_save_header header; + + /* We only take subset of virDomainDefFormat flags. */ + virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL); + + qemuDriverLock(driver); + + fd = qemuDomainSaveImageOpen(driver, path,&def,&header, false, NULL, + NULL, false); + + if (fd< 0) + goto cleanup; + + ret = qemuDomainDefFormatXML(driver, def, flags); + +cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainSaveImageDefineXML(virConnectPtr conn, const char *path, + const char *dxml, unsigned int flags) +{ + struct qemud_driver *driver = conn->privateData; + int ret = -1; + virDomainDefPtr def = NULL; + int fd = -1; + struct qemud_save_header header; + char *xml = NULL; + size_t len; + + virCheckFlags(0, -1); + + qemuDriverLock(driver); + + fd = qemuDomainSaveImageOpen(driver, path,&def,&header, false, NULL, + dxml, true); + + if (fd< 0) { + /* Check for special case of no change needed. */ + if (fd == -2) + ret = 0;
i.e. right here.
+ goto cleanup; + } + + xml = qemuDomainDefFormatXML(driver, def, VIR_DOMAIN_XML_SECURE); + if (!xml) + goto cleanup; + len = strlen(xml) + 1; + + if (len> header.xml_len) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("new xml too large to fit in file")); + }
Did you forget a goto cleanup there?
+ if (VIR_EXPAND_N(xml, len, header.xml_len - len)< 0) {
because if len > header.xml_len, that's going to be a *very large* number :-)
+ virReportOOMError(); + goto cleanup; + }
+ + if (lseek(fd, sizeof(header), SEEK_SET) != sizeof(header)) { + virReportSystemError(errno, _("cannot seek in '%s'"), path); + goto cleanup; + } + if (safewrite(fd, xml, len) != len || + VIR_CLOSE(fd)< 0) { + virReportSystemError(errno, _("failed to write xml to '%s'"), path); + goto cleanup; + } + + ret = 0; + +cleanup: + virDomainDefFree(def); + VIR_FORCE_CLOSE(fd); + VIR_FREE(xml); + qemuDriverUnlock(driver); + return ret; +} + static int qemuDomainObjRestore(virConnectPtr conn, struct qemud_driver *driver, @@ -4009,7 +4111,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL;
fd = qemuDomainSaveImageOpen(driver, path,&def,&header, - bypass_cache,&directFd, NULL); + bypass_cache,&directFd, NULL, false); if (fd< 0) goto cleanup;
@@ -9070,6 +9172,8 @@ static virDriver qemuDriver = { .domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */ .domainRestore = qemuDomainRestore, /* 0.2.0 */ .domainRestoreFlags = qemuDomainRestoreFlags, /* 0.9.4 */ + .domainSaveImageGetXMLDesc = qemuDomainSaveImageGetXMLDesc, /* 0.9.4 */ + .domainSaveImageDefineXML = qemuDomainSaveImageDefineXML, /* 0.9.4 */ .domainCoreDump = qemudDomainCoreDump, /* 0.7.0 */ .domainScreenshot = qemuDomainScreenshot, /* 0.9.2 */ .domainSetVcpus = qemuDomainSetVcpus, /* 0.4.4 */
ACK with the goto cleanup added in when the new length is too big to fit (unless I've misunderstood the code).
On 07/28/2011 12:47 PM, Laine Stump wrote:
On 07/22/2011 12:21 AM, Eric Blake wrote:
The goal here is that save-image-dumpxml fed back to save image-define without changing the save file; anywhere that this is not the case is probably a bug in domain_conf.c.
I'll do a v4 of patch 3/3 to plug those domain_conf.c holes, but in the meantime, this patch is good enough on its own even if the edit cycle is not idempotent yet.
+ if (edit&& STREQ(xml, xmlin)) { + VIR_FREE(xml); + if (VIR_CLOSE(fd)< 0) { + virReportSystemError(errno, _("cannot close file: %s"), path); + goto error; + } + return -2; + }
So "unchanged" is indicated with a < 0 return code, but that only happens when edit == true, and only one of the callers does that (and properly handles the special case).
Yes, because it cannot return a positive value for this case (the function returns an fd on normal success - so one way of looking at it is that no edits needed is a special case failure).
+ + if (len> header.xml_len) { + qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("new xml too large to fit in file")); + }
Did you forget a goto cleanup there?
+ if (VIR_EXPAND_N(xml, len, header.xml_len - len)< 0) {
because if len > header.xml_len, that's going to be a *very large* number :-)
*very large* indeed - I'd love to have a 64-bit machine with that much RAM :-) I've added the missing goto.
ACK with the goto cleanup added in when the new length is too big to fit (unless I've misunderstood the code).
You were right, so I made the change, and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Noticed by comparing the xml generated by virDomainSave with the xml produced by reparsing and redumping that xml. * src/conf/domain_conf.c (virDomainDeviceInfoIsSet): Add parameter, and update all callers. Make static. (virDomainNetDefFormat): Skip generated ifname. * src/conf/domain_conf.h (virDomainDeviceInfoIsSet): Delete. * src/libvirt_private.syms (domain_conf.h): Update. --- Sending this now, to get review started, but I still have some more fixing to do - right now, active domains still include: + <seclabel type='dynamic' model='selinux' relabel='yes'/> which is not present on reparse, but I'm too tired to find out why. src/conf/domain_conf.c | 26 ++++++++++++++++---------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 919a75a..52e8ada 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1388,11 +1388,12 @@ int virDomainDeviceVirtioSerialAddressIsValid( } -int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info) +static int +virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags) { if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) return 1; - if (info->alias) + if (info->alias && !(flags & VIR_DOMAIN_XML_INACTIVE)) return 1; return 0; } @@ -8580,7 +8581,7 @@ virDomainControllerDefFormat(virBufferPtr buf, break; } - if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; @@ -8806,9 +8807,14 @@ virDomainNetDefFormat(virBufferPtr buf, break; } - if (def->ifname) + + if (def->ifname && + !((flags & VIR_DOMAIN_XML_INACTIVE) && + (STRPREFIX(def->ifname, "vnet")))) { + /* Skip auto-generated target names for inactive config. */ virBufferEscapeString(buf, " <target dev='%s'/>\n", def->ifname); + } if (def->model) { virBufferEscapeString(buf, " <model type='%s'/>\n", def->model); @@ -9041,7 +9047,7 @@ virDomainChrDefFormat(virBufferPtr buf, break; } - if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; } @@ -9069,7 +9075,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <smartcard mode='%s'", mode); switch (def->type) { case VIR_DOMAIN_SMARTCARD_TYPE_HOST: - if (!virDomainDeviceInfoIsSet(&def->info)) { + if (!virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, "/>\n"); return 0; } @@ -9119,7 +9125,7 @@ virDomainSoundDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <sound model='%s'", model); - if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; @@ -9148,7 +9154,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <memballoon model='%s'", model); - if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; @@ -9197,7 +9203,7 @@ virDomainWatchdogDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <watchdog model='%s' action='%s'", model, action); - if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; @@ -9280,7 +9286,7 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <input type='%s' bus='%s'", type, bus); - if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 551946b..b97a57a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1388,7 +1388,6 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr); int virDomainDeviceDriveAddressIsValid(virDomainDeviceDriveAddressPtr addr); int virDomainDeviceVirtioSerialAddressIsValid(virDomainDeviceVirtioSerialAddressPtr addr); -int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e7d2579..3996efa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -267,7 +267,6 @@ virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; virDomainDeviceDefFree; virDomainDeviceDefParse; -virDomainDeviceInfoIsSet; virDomainDeviceInfoIterate; virDomainDevicePCIAddressIsValid; virDomainDeviceTypeToString; -- 1.7.4.4
On 07/22/2011 12:21 AM, Eric Blake wrote:
Noticed by comparing the xml generated by virDomainSave with the xml produced by reparsing and redumping that xml.
* src/conf/domain_conf.c (virDomainDeviceInfoIsSet): Add parameter, and update all callers. Make static. (virDomainNetDefFormat): Skip generated ifname. * src/conf/domain_conf.h (virDomainDeviceInfoIsSet): Delete. * src/libvirt_private.syms (domain_conf.h): Update. ---
Sending this now, to get review started, but I still have some more fixing to do - right now, active domains still include:
+<seclabel type='dynamic' model='selinux' relabel='yes'/>
which is not present on reparse, but I'm too tired to find out why.
I know the feeling :-) So does it turn out that this is important, or not?
src/conf/domain_conf.c | 26 ++++++++++++++++---------- src/conf/domain_conf.h | 1 - src/libvirt_private.syms | 1 - 3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 919a75a..52e8ada 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1388,11 +1388,12 @@ int virDomainDeviceVirtioSerialAddressIsValid( }
-int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info) +static int +virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info, unsigned int flags) { if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) return 1; - if (info->alias) + if (info->alias&& !(flags& VIR_DOMAIN_XML_INACTIVE)) return 1; return 0; } @@ -8580,7 +8581,7 @@ virDomainControllerDefFormat(virBufferPtr buf, break; }
- if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1; @@ -8806,9 +8807,14 @@ virDomainNetDefFormat(virBufferPtr buf, break; }
- if (def->ifname) + + if (def->ifname&& + !((flags& VIR_DOMAIN_XML_INACTIVE)&& + (STRPREFIX(def->ifname, "vnet")))) { + /* Skip auto-generated target names for inactive config. */
It's kind of bothersome that use of this magic device name prefix isn't self-contained in domain_conf.c (or somewhere else). Perhaps the string could be defined in domain_conf.h, then used here and in qemu_command.c (is it used any place else?). Aside from that bit of ugliness, ACK. (And I could live with this. at least temporarily, as well, although making all the places work off a single string constant might hypothetically save someone lots of frustration in some uncharted future.)
virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); + } if (def->model) { virBufferEscapeString(buf, "<model type='%s'/>\n", def->model); @@ -9041,7 +9047,7 @@ virDomainChrDefFormat(virBufferPtr buf, break; }
- if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1; } @@ -9069,7 +9075,7 @@ virDomainSmartcardDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<smartcard mode='%s'", mode); switch (def->type) { case VIR_DOMAIN_SMARTCARD_TYPE_HOST: - if (!virDomainDeviceInfoIsSet(&def->info)) { + if (!virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, "/>\n"); return 0; } @@ -9119,7 +9125,7 @@ virDomainSoundDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<sound model='%s'", model);
- if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1; @@ -9148,7 +9154,7 @@ virDomainMemballoonDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<memballoon model='%s'", model);
- if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1; @@ -9197,7 +9203,7 @@ virDomainWatchdogDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<watchdog model='%s' action='%s'", model, action);
- if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1; @@ -9280,7 +9286,7 @@ virDomainInputDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<input type='%s' bus='%s'", type, bus);
- if (virDomainDeviceInfoIsSet(&def->info)) { + if (virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); if (virDomainDeviceInfoFormat(buf,&def->info, flags)< 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 551946b..b97a57a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1388,7 +1388,6 @@ int virDomainDeviceAddressIsValid(virDomainDeviceInfoPtr info, int virDomainDevicePCIAddressIsValid(virDomainDevicePCIAddressPtr addr); int virDomainDeviceDriveAddressIsValid(virDomainDeviceDriveAddressPtr addr); int virDomainDeviceVirtioSerialAddressIsValid(virDomainDeviceVirtioSerialAddressPtr addr); -int virDomainDeviceInfoIsSet(virDomainDeviceInfoPtr info); void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info); void virDomainDefClearPCIAddresses(virDomainDefPtr def); void virDomainDefClearDeviceAliases(virDomainDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e7d2579..3996efa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -267,7 +267,6 @@ virDomainDeviceAddressIsValid; virDomainDeviceAddressTypeToString; virDomainDeviceDefFree; virDomainDeviceDefParse; -virDomainDeviceInfoIsSet; virDomainDeviceInfoIterate; virDomainDevicePCIAddressIsValid; virDomainDeviceTypeToString;
On 07/28/2011 12:59 PM, Laine Stump wrote:
On 07/22/2011 12:21 AM, Eric Blake wrote:
Noticed by comparing the xml generated by virDomainSave with the xml produced by reparsing and redumping that xml.
* src/conf/domain_conf.c (virDomainDeviceInfoIsSet): Add parameter, and update all callers. Make static. (virDomainNetDefFormat): Skip generated ifname. * src/conf/domain_conf.h (virDomainDeviceInfoIsSet): Delete. * src/libvirt_private.syms (domain_conf.h): Update. ---
Sending this now, to get review started, but I still have some more fixing to do - right now, active domains still include:
+<seclabel type='dynamic' model='selinux' relabel='yes'/>
which is not present on reparse, but I'm too tired to find out why.
I know the feeling :-)
Now that I've had some sleep (and 6 days have elapsed), I've finally gotten back to this patch. :-)
So does it turn out that this is important, or not?
It _would_ be, if we cared about non-empty model on inactive parse. That is, if we _wanted_ to force a dynamic security model of selinux instead of apparmor, then the inactive parse needs to be taught to parse model, and enforce that the model is supported by the current host (and prevent migrations between selinux and apparmor machines). But since that particular <seclabel> merely represents the default, and by default you want a secure machine regardless of which security model your host supports, I simply fixed the formatter to omit default information rather than teaching the parser to honor an explicit model (that is, existing behavior has always been to ignore model on inactive parse).
+ + if (def->ifname&& + !((flags& VIR_DOMAIN_XML_INACTIVE)&& + (STRPREFIX(def->ifname, "vnet")))) { + /* Skip auto-generated target names for inactive config. */
It's kind of bothersome that use of this magic device name prefix isn't self-contained in domain_conf.c (or somewhere else). Perhaps the string could be defined in domain_conf.h, then used here and in qemu_command.c (is it used any place else?).
Split into a separate patch - uml_conf.c also used it. v4 now posted, and my audit of domain_conf.c is now complete. https://www.redhat.com/archives/libvir-list/2011-July/msg02064.html -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/22/2011 12:21 AM, Eric Blake wrote:
With this, it is possible to update the path to a disk backing image on either the save or restore action, without having to binary edit the XML embedded in the state file.
This also modifies virDomainSave to output a smaller xml (only the inactive xml, which is all the more virDomainRestore parses), while still guaranteeing padding for most typical abi-compatible xml replacements, necessary so that the next patch for virDomainSaveImageDefineXML will not cause unnecessary modifications to the save image file.
* src/qemu/qemu_driver.c (qemuDomainSaveInternal): Add parameter, only use inactive state, and guarantee padding. (qemuDomainSaveImageOpen): Add parameter. (qemuDomainSaveFlags, qemuDomainManagedSave) (qemuDomainRestoreFlags, qemuDomainObjRestore): Update callers. ---
v3: change virDomainSave to always output minimal information, but with fixed padding added, so that save file modification will always be more likely to succeed,
"always be more likely". Heh. Looking at this problem from the outside, it seems that if we wanted a 100% reliable solution, we would need to introduce the idea of a linked header, which can be continued at the end of the file (of course that wouldn't work if there are ever cases where the file is being read from a pipe, and we can't seek, and it's entirely possible that 1024 is always enough extra to ensure everything works).
and not generate quite as many xml differences on round trips.
src/qemu/qemu_driver.c | 109 +++++++++++++++++++++++++++++------------------ 1 files changed, 67 insertions(+), 42 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1401967..2b1df6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2193,7 +2193,7 @@ qemuCompressProgramName(int compress) static int qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, virDomainObjPtr vm, const char *path, - int compressed, bool bypass_cache) + int compressed, bool bypass_cache, const char *xmlin) { char *xml = NULL; struct qemud_save_header header; @@ -2204,7 +2204,9 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, qemuDomainObjPrivatePtr priv; struct stat sb; bool is_reg = false; + size_t len; unsigned long long offset; + unsigned long long pad; int fd = -1; uid_t uid = getuid(); gid_t gid = getgid(); @@ -2239,15 +2241,54 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } }
- /* Get XML for the domain */ - xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE); + /* Get XML for the domain. Restore needs only the inactive xml, + * including secure. We should get the same result whether xmlin + * is NULL or whether it was the live xml of the domain moments + * before. */ + if (xmlin) { + virDomainDefPtr def = NULL; + + if (!(def = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) { + goto endjob; + } + if (!virDomainDefCheckABIStability(vm->def, def)) { + virDomainDefFree(def); + goto endjob; + } + xml = virDomainDefFormat(def, VIR_DOMAIN_XML_SECURE); + } else { + xml = virDomainDefFormat(vm->def, (VIR_DOMAIN_XML_INACTIVE | + VIR_DOMAIN_XML_SECURE)); + } if (!xml) { qemuReportError(VIR_ERR_OPERATION_FAILED, "%s", _("failed to get domain xml")); goto endjob; } - header.xml_len = strlen(xml) + 1; + len = strlen(xml) + 1; + offset = sizeof(header) + len; + + /* Due to way we append QEMU state on our header with dd, + * we need to ensure there's a 512 byte boundary. Unfortunately + * we don't have an explicit offset in the header, so we fake + * it by padding the XML string with NUL bytes. Additionally, + * we want to ensure that virDomainSaveImageDefineXML can supply + * slightly larger XML, so we add a miminum padding prior to + * rounding out to page boundaries. + */ + pad = 1024; + pad += (QEMU_MONITOR_MIGRATE_TO_FILE_BS - + ((offset + pad) % QEMU_MONITOR_MIGRATE_TO_FILE_BS)); + if (VIR_EXPAND_N(xml, len, pad)< 0) { + virReportOOMError(); + goto endjob; + } + offset += pad; + header.xml_len = len;
+ /* Obtain the file handle. */ /* path might be a pre-existing block dev, in which case * we need to skip the create step, and also avoid unlink * in the failure case */ @@ -2268,29 +2309,6 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom, } }
- offset = sizeof(header) + header.xml_len; - - /* Due to way we append QEMU state on our header with dd, - * we need to ensure there's a 512 byte boundary. Unfortunately - * we don't have an explicit offset in the header, so we fake - * it by padding the XML string with NULLs. - */ - if (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS) { - unsigned long long pad = - QEMU_MONITOR_MIGRATE_TO_FILE_BS - - (offset % QEMU_MONITOR_MIGRATE_TO_FILE_BS); - - if (VIR_REALLOC_N(xml, header.xml_len + pad)< 0) { - virReportOOMError(); - goto endjob; - } - memset(xml + header.xml_len, 0, pad); - offset += pad; - header.xml_len += pad; - } - - /* Obtain the file handle. */ - /* First try creating the file as root */ if (bypass_cache) { directFlag = virFileDirectFdFlag(); @@ -2461,11 +2479,6 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, virDomainObjPtr vm = NULL;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); - if (dxml) { - qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("xml modification unsupported")); - return -1; - }
qemuDriverLock(driver);
@@ -2503,7 +2516,8 @@ qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, }
ret = qemuDomainSaveInternal(driver, dom, vm, path, compressed, - (flags& VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); + (flags& VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + dxml); vm = NULL;
cleanup: @@ -2567,7 +2581,8 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
compressed = QEMUD_SAVE_FORMAT_RAW; ret = qemuDomainSaveInternal(driver, dom, vm, name, compressed, - (flags& VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0); + (flags& VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, + NULL); vm = NULL;
cleanup: @@ -3696,7 +3711,8 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, const char *path, virDomainDefPtr *ret_def, struct qemud_save_header *ret_header, - bool bypass_cache, virFileDirectFdPtr *directFd) + bool bypass_cache, virFileDirectFdPtr *directFd, + const char *xmlin) { int fd; struct qemud_save_header header; @@ -3780,6 +3796,20 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver, QEMU_EXPECTED_VIRT_TYPES, VIR_DOMAIN_XML_INACTIVE)))
(I've never noticed before that the diff shows the old signature of a function if its signature has been changed as a part of the current patch. A bit confusing for the human, but I'm sure doing it the other way would make it confusing for the machine :-)
goto error; + if (xmlin) { + virDomainDefPtr def2 = NULL; + + if (!(def2 = virDomainDefParseString(driver->caps, xmlin, + QEMU_EXPECTED_VIRT_TYPES, + VIR_DOMAIN_XML_INACTIVE))) + goto error; + if (!virDomainDefCheckABIStability(def, def2)) { + virDomainDefFree(def2); + goto error; + } + virDomainDefFree(def); + def = def2; + }
VIR_FREE(xml);
@@ -3914,17 +3944,12 @@ qemuDomainRestoreFlags(virConnectPtr conn, virFileDirectFdPtr directFd = NULL;
virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE, -1); - if (dxml) { - qemuReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("xml modification unsupported")); - return -1; - }
qemuDriverLock(driver);
fd = qemuDomainSaveImageOpen(driver, path,&def,&header, (flags& VIR_DOMAIN_SAVE_BYPASS_CACHE) != 0, -&directFd); +&directFd, dxml); if (fd< 0) goto cleanup;
@@ -3984,7 +4009,7 @@ qemuDomainObjRestore(virConnectPtr conn, virFileDirectFdPtr directFd = NULL;
fd = qemuDomainSaveImageOpen(driver, path,&def,&header, - bypass_cache,&directFd); + bypass_cache,&directFd, NULL); if (fd< 0) goto cleanup;
ACK.
On 07/28/2011 12:00 PM, Laine Stump wrote:
---
v3: change virDomainSave to always output minimal information, but with fixed padding added, so that save file modification will always be more likely to succeed,
"always be more likely". Heh.
I guess dropping "always" would make that sentence more believable.
Looking at this problem from the outside, it seems that if we wanted a 100% reliable solution, we would need to introduce the idea of a linked header, which can be continued at the end of the file (of course that wouldn't work if there are ever cases where the file is being read from a pipe, and we can't seek, and it's entirely possible that 1024 is always enough extra to ensure everything works).
We hand the file over to qemu after seeking to the end of our header, and I don't think qemu tolerates garbage at the end of its saved state files (qemu only reads in the saved state as if by a pipe; in reality the saved state file is the same as what gets sent over a socket during migration, which really is a one-pass non-seeking algorithm). So a split header won't really work. But hopefully there's not too many ABI compatible changes that you can make that significantly increase the size of the XML. And in the worst case, you can always fall back to: virsh restore file --xml alternate in the case where virsh save-image-define file alternate complains about a too-large alternate.
ACK.
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump