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.  The
bulk flag requires no pre-existing snapshot metadata (as that
makes life much easier if there is a failure encountered partway
through the list processing - simply remove all other snapshot
metadatas), and makes no guarantees on which snapshot (when there
are multiple) will actually be returned.

Actual driver implementations will be in later patches.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
---
 include/libvirt/libvirt-domain-snapshot.h |  3 +++
 src/libvirt-domain-snapshot.c             | 23 +++++++++++++++++++----
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index d9b689abbd..7e6eff6a9a 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -71,6 +71,9 @@ typedef enum {
     VIR_DOMAIN_SNAPSHOT_CREATE_LIVE        = (1 << 8), /* create the snapshot
                                                           while the guest is
                                                           running */
+    VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST = (1 << 9), /* parse multiple
+                                                            snapshots in one
+                                                            API call */
 } virDomainSnapshotCreateFlags;

 /* Take a snapshot of the current VM state */
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index a647a500d6..ab94d29574 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -1,7 +1,7 @@
 /*
  * libvirt-domain-snapshot.c: entry points for virDomainSnapshotPtr APIs
  *
- * Copyright (C) 2006-2014 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -102,7 +102,7 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
  * @flags: bitwise-OR of virDomainSnapshotCreateFlags
  *
  * Creates a new snapshot of a domain based on the snapshot xml
- * contained in xmlDesc.
+ * contained in xmlDesc, with a top-level <domainsnapshot> element.
  *
  * If @flags is 0, the domain can be active, in which case the
  * snapshot will be a full system snapshot (capturing both disk state,
@@ -132,8 +132,17 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
  * guest-visible layout.  When redefining a snapshot name that does
  * not exist, the hypervisor may validate that reverting to the
  * snapshot appears to be possible (for example, disk images have
- * snapshot contents by the requested name).  Not all hypervisors
- * support these flags.
+ * snapshot contents by the requested name).  Alternatively, if @flags
+ * includes VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST (which requires
+ * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE and is incompatible with
+ * VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT), and the domain has no existing
+ * snapshot metadata, then @xmlDesc is parsed as a top-level
+ * <snapshots> element with an optional current='name' attribute, and
+ * containing one or more <domainsnapshot> children (as produced by
+ * virDomainGetXMLDesc() with the flag VIR_DOMAIN_XML_SNAPSHOTS), to
+ * do a bulk redefine of all snapshots at once (it is unspecified
+ * which of the redefined snapshots will be used as the return value
+ * on success).  Not all hypervisors support these flags.
  *
  * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, then the
  * domain's disk images are modified according to @xmlDesc, but then
@@ -219,6 +228,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
     VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT,
                           VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE,
                           error);
+    VIR_REQUIRE_FLAG_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST,
+                          VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE,
+                          error);

     VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE,
                              VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA,
@@ -226,6 +238,9 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
     VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE,
                              VIR_DOMAIN_SNAPSHOT_CREATE_HALT,
                              error);
+    VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE_LIST,
+                             VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT,
+                             error);

     if (conn->driver->domainSnapshotCreateXML) {
         virDomainSnapshotPtr ret;
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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?

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.

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 */
}

Jano

>The
>bulk flag requires no pre-existing snapshot metadata (as that
>makes life much easier if there is a failure encountered partway
>through the list processing - simply remove all other snapshot
>metadatas), and makes no guarantees on which snapshot (when there
>are multiple) will actually be returned.
>
>Actual driver implementations will be in later patches.
>
>Signed-off-by: Eric Blake <eblake@redhat.com>
>Reviewed-by: John Ferlan <jferlan@redhat.com>
>---
> include/libvirt/libvirt-domain-snapshot.h |  3 +++
> src/libvirt-domain-snapshot.c             | 23 +++++++++++++++++++----
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list