Re: [libvirt] [PATCH 20/16] save: let qemu driver manipulate save files
On 07/21/2011 05:47 AM, Daniel P. Berrange wrote:
On Wed, Jul 20, 2011 at 02:54:26PM -0600, Eric Blake wrote:
> * src/qemu/qemu_driver.c (qemuDomainSaveImageGetXMLDesc)
> (qemuDomainSaveImageDefineXML): New functions.
> (qemuDomainSaveImageOpen): Add parameter.
> (qemuDomainRestoreFlags, qemuDomainObjRestore): Adjust clients.
> (qemuDomainSaveInternal): Simplify array expansion.
> ---
> + 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"));
> + }
This is what I was afraid of when I saw the API proposal. I fear that
this could make the use of the new API rather limited. I can easily
imagine 50%+ of end users wanting to the change the save image XML by
altering a disk path and getting a disk path which is longer.
We have one thing in our favor:
By default, virDomainSave stores the _active_ XML, rounded up to the
next 4k boundary. But virDomainRestore only reads the _inactive_ XML.
So my patch explicitly writes the inactive XML, which is typically much
shorter than the active XML that was there originally (no <alias> tags,
and so forth) - longer disk names are not a problem if they are still
shorter than the length of the no-longer-present <alias> tags. To
trigger this out-of-length error without breaking ABI, you have to try
_really_ hard, such as making <description> much longer.
I don't think we should add an API like this unless we can come
up
with a plan for addressing this problem which is generally going to
work.
For in-place edits, I hold the driver lock for the entire time, so there
is no chance that any other virDomainRestore can be called and see the
state file in an inconsistent state.
But if we want to accommodate larger xml lengths, in spite of my above
claim that we usually won't encounter them because of the difference in
live vs. inactive shortening the length on average, then I think we have to:
mark the original file as in-use
drop driver lock
write the new longer header into a temp file
copy the old qemu data into the new file at the new offset
regain driver lock
rename the temp file over the original file
since the copy operation will be long-lived (no longer just 4k or 8k,
but multiple megabytes, depending on how large the original save image
was). Dropping the driver lock in the meantime means that another
thread can call virDomainRestore using the same original file, which
must fail because we are in the middle of altering it, hence the
importance of being able to mark the original file in-use.
Thoughts on how to proceed?
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org