On 3/7/19 10:13 AM, Ján Tomko wrote:
On Mon, Mar 04, 2019 at 09:34:40PM -0600, Eric Blake wrote:
Continue the work of the previous patch in making it possible to copy the state of a transient domain with snapshots from one host to another, by allowing the destination to perform bulk redefines. Note that the destination still has to do separate calls for creating/defining the domain first, and then redefining the snapshots (that is, there is intentional asymmetry between dumping the list in virDomainGetXMLDesc() but redefining it via virDomainSnapshotCreateXML()), but this is better than the previous state of having to make multiple REDEFINE calls.
What is the intention behind the assymetry?
virDomainSnapshotGetXMLDesc won't work - you can't pass in NULL because that function requires a snapshot (in order to get at the virDomain and virConnection) to even make the call. On the flip side, I did NOT want virDomainDefine/virDomainCreate to take the <snapshots> argument, even with the presence of a flag, because there are scenarios where you want the domain defined before you add in the snapshots; virDomainSnapshotCreateXML with new flag fit that purpose well. I could, however, add a new API instead of a new flag overloading to the existing API. Naming is hard, maybe: virDomainGetSnapshotsXMLDesc since it would be a new virDomain function, but returns the new <snapshots> XML element.
It feels odd to request the list by a flag to virDomainGetXMLDesc (because we're not going to reuse the whole <domain>, just the <snapshots> sub-element here). Having a counterpart to the API doing the redefine would only mean two calls (GetXMLDesc, GetSnapshots) instead of getting every snapshot separately.
One call (virDomainGetXMLDesc with flag) is even better than two (virDomainGetXMLDesc, virDomainGetSnapshotsXMLDesc), but a new API has the benefit of not suffering from the recently-fixed bug about unknown new flags being rejected by buggy old servers.
Also, virDomainSnapshotCreateXML is designed for a single snapshot, using a flag to turn it into a different API ('virDomainSnapshotsCreateXML'? 'virDomainSnapshotsRedefine'?) leads to strangeness like returning a single snapshot while making no guarantees on which one it is or a repetition of this pattern: if (flags & REDEFINE_LIST) { /* ... */ goto cleanup; /* <- no fallthrough here */ }
If I add a new API for getting the XML, then it is not a stretch to require a new API for redefining all snapshots at once. And now that I've typed this up, the suggestion for a separate API is starting to be more appealing. Looks like I'll be posting a v4 shortly. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org