[libvirt] [PATCHv3 1/3] save: support qemu modifying xml on domain save/restore
Laine Stump
laine at laine.org
Thu Jul 28 18:00:37 UTC 2011
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.
More information about the libvir-list
mailing list