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 ebl...@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list