On 11/20/2012 09:41 AM, Peter Krempa wrote:
>> @@ -188,6 +188,7 @@ virDomainSnapshotDefParseString(const char *xmlStr,
>> char *memorySnapshot = NULL;
>> char *memoryFile = NULL;
>> bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE);
>> + virDomainSnapshotObjPtr other = NULL;
>
> I'd also set "tmp" to NULL here and ...
>
>>
>> xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt);
>> if (!xml) {
>> @@ -223,7 +224,61 @@ virDomainSnapshotDefParseString(const char *xmlStr,
>> + tmp = virXPathString("string(./branch)", ctxt);
>> + if (!tmp) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("use of branch flag requires <branch>
>> element"));
>> + goto cleanup;
>> + }
>> + other = virDomainSnapshotFindByName(snapshots, tmp);
>> + if (!other) {
>> + virReportError(VIR_ERR_XML_ERROR, _("could not find
>> branch '%s'"),
>> + tmp);
>> + VIR_FREE(tmp);
>
> move this free statement right to the cleanup section.
Good idea. Also, it will let me raise an error if the user passed
<branch> in the xml but did not pass the right flag.
>> + if (other) {
>> + /* XXX It would be nice to allow automatic reuse of existing
>> + * memory files, with bookkeeping that prevents deleting a
>> + * file if some other branch is still using it. But for a
>> + * first implementation, it is easier to enforce that the user
>> + * always matches (disk-only branching to disk-only;
>> + * checkpoint branching to checkpoint and giving us a new name
>> + * which we set up as a copy). There is also a question of
>> + * whether attempting a hard link rather than always copying
>> + * to a new inode makes sense, if the original is a regular
>> + * file and not a block device. */
>
> Hm, despite this was my idea it's looking more strange the more I think
> about it. If we're going to have the users to specify a new image file
> now we will need to support that after we come up with a better version.
> I'm going to think about this a bit more. But for now it seems to be the
> laziest solution.
Actually, now that I'm diving into qemu implementation, it is actually
harder to allow (or even require) the user to pass the external file
name; in order to link() or copy the original file into the new name,
the qemu driver has to know what the old name was. So I either have to
alter this function signature to return one more piece of information,
or else explicitly plan that any code we do for deleting a snapshot is
careful to not delete the external memory file if the snapshot has
siblings. Also, while link() is fast, copying is potentially slow (and
since state files can be several hundred megabytes, doing a copy can
also potentially wreak havoc on the file system cache unless we use
O_DIRECT, and I really don't want to go there). So in my next revision
of the series, I'm going with the shared file approach, as it just seems
cleaner all around.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list