[libvirt] [PATCH v4 1/8] snapshot: Add new API for bulk dumpxml/redefine

Eric Blake posted 8 patches 6 years, 11 months ago
[libvirt] [PATCH v4 1/8] snapshot: Add new API for bulk dumpxml/redefine
Posted by Eric Blake 6 years, 11 months ago
Right now, copying the state of a transient domain with snapshots from
one host to another requires multiple API calls on both machines - on
the host: get the domain XML, get a list of the snapshots, and then
for each snapshot get the snapshot's XML; then on the destination:
create the domain, then multiple domain snapshot create calls (must
be in topological order) with the REDEFINE flag. This patch aims to
make the process use fewer APIs by making it possible to grab the XML
for all snapshots at once on the source, then redefine from that same
list at the destination. In fact, although it is easy to match recent
changes to allow output in topological order, it is also easy to do
bulk redefine in arbitrary order.

Consideration was given to adding a flag to existing APIs; but to do
that, there would be an asymmetry of dumping <snapshots> as a
sub-element of <domain> during virDomainGetXMLDesc(), vs. redefining
<snapshots> as a top-level element (and different than the normal
top-level element) of virDomainSnapshotCreateXML(). Thus, it is
cleaner to have two new APIs:

virDomainGetSnapshotsXMLDesc
virDomainImportSnapshotsXML

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/libvirt/libvirt-domain-snapshot.h |  16 ++-
 src/driver-hypervisor.h                   |  13 ++-
 src/libvirt-domain-snapshot.c             | 122 +++++++++++++++++++++-
 src/libvirt_public.syms                   |   2 +
 4 files changed, 148 insertions(+), 5 deletions(-)

diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index 602e5def59..7c2e001b87 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -3,7 +3,7 @@
  * Summary: APIs for management of domain snapshots
  * Description: Provides APIs for the management of domain snapshots
  *
- * 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
@@ -78,6 +78,11 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain,
                                                 const char *xmlDesc,
                                                 unsigned int flags);

+/* Bulk import a list of snapshots */
+int virDomainImportSnapshotsXML(virDomainPtr domain,
+                                const char *xmlDesc,
+                                unsigned int flags);
+
 typedef enum {
     VIR_DOMAIN_SNAPSHOT_XML_SECURE         = VIR_DOMAIN_XML_SECURE, /* dump security sensitive information too */
 } virDomainSnapshotXMLFlags;
@@ -86,6 +91,15 @@ typedef enum {
 char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
                                   unsigned int flags);

+typedef enum {
+    VIR_DOMAIN_GET_SNAPSHOTS_XML_SECURE         = (1 << 0), /* dump security sensitive information too */
+    VIR_DOMAIN_GET_SNAPSHOTS_XML_TOPOLOGICAL    = (1 << 1), /* ensure parents occur before children */
+} virDomainGetSnapshotsXMLFlags;
+
+/* Dump the XML of all snapshots */
+char *virDomainGetSnapshotsXMLDesc(virDomainPtr domain,
+                                   unsigned int flags);
+
 /**
  * virDomainSnapshotListFlags:
  *
diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
index 5315e33dde..c30e16b690 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -1,7 +1,7 @@
 /*
  * driver-hypervisor.h: entry points for hypervisor drivers
  *
- * Copyright (C) 2006-2015 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
@@ -797,10 +797,19 @@ typedef virDomainSnapshotPtr
                                  const char *xmlDesc,
                                  unsigned int flags);

+typedef int
+(*virDrvDomainImportSnapshotsXML)(virDomainPtr domain,
+                                  const char *xmlDesc,
+                                  unsigned int flags);
+
 typedef char *
 (*virDrvDomainSnapshotGetXMLDesc)(virDomainSnapshotPtr snapshot,
                                   unsigned int flags);

+typedef char *
+(*virDrvDomainGetSnapshotsXMLDesc)(virDomainPtr domain,
+                                   unsigned int flags);
+
 typedef int
 (*virDrvDomainSnapshotNum)(virDomainPtr domain,
                            unsigned int flags);
@@ -1494,7 +1503,9 @@ struct _virHypervisorDriver {
     virDrvDomainManagedSaveGetXMLDesc domainManagedSaveGetXMLDesc;
     virDrvDomainManagedSaveDefineXML domainManagedSaveDefineXML;
     virDrvDomainSnapshotCreateXML domainSnapshotCreateXML;
+    virDrvDomainImportSnapshotsXML domainImportSnapshotsXML;
     virDrvDomainSnapshotGetXMLDesc domainSnapshotGetXMLDesc;
+    virDrvDomainGetSnapshotsXMLDesc domainGetSnapshotsXMLDesc;
     virDrvDomainSnapshotNum domainSnapshotNum;
     virDrvDomainSnapshotListNames domainSnapshotListNames;
     virDrvDomainListAllSnapshots domainListAllSnapshots;
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index d133c84933..1ad8dfbfb8 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -133,7 +133,8 @@ virDomainSnapshotGetConnect(virDomainSnapshotPtr snapshot)
  * 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.
+ * support these flags; and some hypervisors support
+ * virDomainImportSnapshotsXML() for redefining all metadata in one call.
  *
  * If @flags includes VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, then the
  * domain's disk images are modified according to @xmlDesc, but then
@@ -242,6 +243,65 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
 }


+/**
+ * virDomainImportSnapshotsXML:
+ * @domain: a domain object
+ * @xmlDesc: string containing an XML description of a list of domain snapshots
+ * @flags: extra flags; not used yet, so callers should always pass 0
+ *
+ * Imports the metadata for a list of domain snapshots using
+ * @xmlDesc with a top-level element of <snapshots>.
+ *
+ * This call requires that the domain currently has no snapshot
+ * metadata, and the snapshots can be listed in any order, whereas
+ * using virDomainSnapshotCreateXML() with its
+ * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE flag requires multiple calls
+ * and topological sorting. Bulk redefinition is mainly useful for
+ * reinstating metadata in situations such as transient domains or
+ * migration.
+ *
+ * Generally, the list of snapshots is obtained from
+ * virDomainGetSnapshotsXMLDesc() prior to a scenario that requires
+ * removing snapshot metadata (such as virDomainUndefine() on a
+ * transient domain); although it could also be constructed by
+ * concatenating virDomainSnapshotGetXMLDesc() for each snapshot and
+ * wrapping with a <snapshots> element and optional attribute
+ * current='name' pointing to the current snapshot.
+ *
+ * Returns a count of snapshots imported on success, or -1 on failure.
+ */
+int
+virDomainImportSnapshotsXML(virDomainPtr domain,
+                            const char *xmlDesc,
+                            unsigned int flags)
+{
+    virConnectPtr conn;
+
+    VIR_DOMAIN_DEBUG(domain, "xmlDesc=%s, flags=0x%x", xmlDesc, flags);
+
+    virResetLastError();
+
+    virCheckDomainReturn(domain, -1);
+    conn = domain->conn;
+
+    virCheckNonNullArgGoto(xmlDesc, error);
+    virCheckReadOnlyGoto(conn->flags, error);
+
+    if (conn->driver->domainImportSnapshotsXML) {
+        int ret = conn->driver->domainImportSnapshotsXML(domain, xmlDesc,
+                                                         flags);
+        if (ret < 0)
+            goto error;
+        return ret;
+    }
+
+    virReportUnsupportedError();
+ error:
+    virDispatchError(conn);
+    return -1;
+}
+
+
 /**
  * virDomainSnapshotGetXMLDesc:
  * @snapshot: a domain snapshot object
@@ -254,8 +314,8 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
  * VIR_DOMAIN_SNAPSHOT_XML_SECURE; this flag is rejected on read-only
  * connections.
  *
- * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error.
- *         the caller must free() the returned value.
+ * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case
+ * of error.  The caller must free() the returned value.
  */
 char *
 virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
@@ -291,6 +351,62 @@ virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
 }


+/**
+ * virDomainGetSnapshotsXMLDesc:
+ * @domain: a domain object
+ * @flags: bitwise-OR of virDomainGetSnapshotsXMLFlags
+ *
+ * Provide an XML description of all domain snapshots, with a top-level
+ * element of <snapshots>.
+ *
+ * No security-sensitive data will be included unless @flags contains
+ * VIR_DOMAIN_GET_SNAPSHOTS_XML_SECURE; this flag is rejected on read-only
+ * connections.
+ *
+ * If @flags contains VIR_DOMAIN_GET_SNAPSHOTS_XML_TOPOLOGICAL, then
+ * it is guaranteed that no snapshot appears in the resulting XML
+ * prior to its parent; otherwise, the order of snapshots in the
+ * resulting list is unspecified. Use of this flag is not required for
+ * virDomainImportSnapshotsXML() to transfer snapshot metadata to
+ * another instance of libvirt.
+ *
+ * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case
+ * of error.  The caller must free() the returned value.
+ */
+char *
+virDomainGetSnapshotsXMLDesc(virDomainPtr domain,
+                             unsigned int flags)
+{
+    virConnectPtr conn;
+    VIR_DOMAIN_DEBUG(domain, "flags=0x%x", flags);
+
+    virResetLastError();
+
+    virCheckDomainReturn(domain, NULL);
+    conn = domain->conn;
+
+    if ((conn->flags & VIR_CONNECT_RO) &&
+        (flags & VIR_DOMAIN_GET_SNAPSHOTS_XML_SECURE)) {
+        virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+                       _("virDomainGetSnapshotsXMLDesc with secure flag"));
+        goto error;
+    }
+
+    if (conn->driver->domainGetSnapshotsXMLDesc) {
+        char *ret;
+        ret = conn->driver->domainGetSnapshotsXMLDesc(domain, flags);
+        if (!ret)
+            goto error;
+        return ret;
+    }
+
+    virReportUnsupportedError();
+ error:
+    virDispatchError(conn);
+    return NULL;
+}
+
+
 /**
  * virDomainSnapshotNum:
  * @domain: a domain object
diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms
index dbce3336d5..d40da8b893 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -817,6 +817,8 @@ LIBVIRT_4.10.0 {
 LIBVIRT_5.2.0 {
     global:
         virConnectGetStoragePoolCapabilities;
+        virDomainGetSnapshotsXMLDesc;
+        virDomainImportSnapshotsXML;
 } LIBVIRT_4.10.0;

 # .... define new API here using predicted next version number ....
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/8] snapshot: Add new API for bulk dumpxml/redefine
Posted by Ján Tomko 6 years, 11 months ago
On Mon, Mar 11, 2019 at 09:38:32PM -0500, Eric Blake wrote:
>Right now, copying the state of a transient domain with snapshots from
>one host to another requires multiple API calls on both machines - on
>the host: get the domain XML, get a list of the snapshots, and then
>for each snapshot get the snapshot's XML; then on the destination:
>create the domain, then multiple domain snapshot create calls (must
>be in topological order) with the REDEFINE flag. This patch aims to
>make the process use fewer APIs by making it possible to grab the XML
>for all snapshots at once on the source, then redefine from that same
>list at the destination. In fact, although it is easy to match recent
>changes to allow output in topological order, it is also easy to do
>bulk redefine in arbitrary order.
>
>Consideration was given to adding a flag to existing APIs; but to do
>that, there would be an asymmetry of dumping <snapshots> as a
>sub-element of <domain> during virDomainGetXMLDesc(), vs. redefining
><snapshots> as a top-level element (and different than the normal
>top-level element) of virDomainSnapshotCreateXML(). Thus, it is
>cleaner to have two new APIs:
>
>virDomainGetSnapshotsXMLDesc
>virDomainImportSnapshotsXML

These do not share a common prefix, but that makes them look nicer IMO.

>
>Signed-off-by: Eric Blake <eblake@redhat.com>
>---
> include/libvirt/libvirt-domain-snapshot.h |  16 ++-
> src/driver-hypervisor.h                   |  13 ++-
> src/libvirt-domain-snapshot.c             | 122 +++++++++++++++++++++-
> src/libvirt_public.syms                   |   2 +
> 4 files changed, 148 insertions(+), 5 deletions(-)
>
>diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
>index 602e5def59..7c2e001b87 100644
>--- a/include/libvirt/libvirt-domain-snapshot.h
>+++ b/include/libvirt/libvirt-domain-snapshot.h
>@@ -3,7 +3,7 @@
>  * Summary: APIs for management of domain snapshots
>  * Description: Provides APIs for the management of domain snapshots
>  *
>- * Copyright (C) 2006-2014 Red Hat, Inc.
>+ * Copyright (C) 2006-2019 Red Hat, Inc.

Do we actually care about updating these?
But an include file will possibly be read by people without git access.

>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public

[...]

>@@ -242,6 +243,65 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
> }
>
>
>+/**
>+ * virDomainImportSnapshotsXML:
>+ * @domain: a domain object
>+ * @xmlDesc: string containing an XML description of a list of domain snapshots
>+ * @flags: extra flags; not used yet, so callers should always pass 0
>+ *
>+ * Imports the metadata for a list of domain snapshots using
>+ * @xmlDesc with a top-level element of <snapshots>.
>+ *
>+ * This call requires that the domain currently has no snapshot
>+ * metadata, and the snapshots can be listed in any order, whereas
>+ * using virDomainSnapshotCreateXML() with its
>+ * VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE flag requires multiple calls
>+ * and topological sorting. Bulk redefinition is mainly useful for
>+ * reinstating metadata in situations such as transient domains or
>+ * migration.
>+ *
>+ * Generally, the list of snapshots is obtained from
>+ * virDomainGetSnapshotsXMLDesc() prior to a scenario that requires
>+ * removing snapshot metadata (such as virDomainUndefine() on a
>+ * transient domain); although it could also be constructed by
>+ * concatenating virDomainSnapshotGetXMLDesc() for each snapshot and
>+ * wrapping with a <snapshots> element and optional attribute
>+ * current='name' pointing to the current snapshot.
>+ *
>+ * Returns a count of snapshots imported on success, or -1 on failure.

This says nothing about partial success, i.e. whether the count returned
can be different from the number of snapshots present in the file.

>+ */
>+int
>+virDomainImportSnapshotsXML(virDomainPtr domain,
>+                            const char *xmlDesc,
>+                            unsigned int flags)
>+{
>+    virConnectPtr conn;
>+
>+    VIR_DOMAIN_DEBUG(domain, "xmlDesc=%s, flags=0x%x", xmlDesc, flags);
>+
>+    virResetLastError();
>+
>+    virCheckDomainReturn(domain, -1);
>+    conn = domain->conn;
>+
>+    virCheckNonNullArgGoto(xmlDesc, error);
>+    virCheckReadOnlyGoto(conn->flags, error);
>+
>+    if (conn->driver->domainImportSnapshotsXML) {
>+        int ret = conn->driver->domainImportSnapshotsXML(domain, xmlDesc,
>+                                                         flags);
>+        if (ret < 0)
>+            goto error;
>+        return ret;
>+    }
>+
>+    virReportUnsupportedError();
>+ error:
>+    virDispatchError(conn);
>+    return -1;
>+}
>+
>+
> /**
>  * virDomainSnapshotGetXMLDesc:
>  * @snapshot: a domain snapshot object
>@@ -254,8 +314,8 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
>  * VIR_DOMAIN_SNAPSHOT_XML_SECURE; this flag is rejected on read-only
>  * connections.
>  *
>- * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error.
>- *         the caller must free() the returned value.
>+ * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case
>+ * of error.  The caller must free() the returned value.

Unrelated change.

>  */
> char *
> virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v4 1/8] snapshot: Add new API for bulk dumpxml/redefine
Posted by Eric Blake 6 years, 11 months ago
On 3/12/19 7:57 AM, Ján Tomko wrote:
> On Mon, Mar 11, 2019 at 09:38:32PM -0500, Eric Blake wrote:
>> Right now, copying the state of a transient domain with snapshots from
>> one host to another requires multiple API calls on both machines - on
>> the host: get the domain XML, get a list of the snapshots, and then
>> for each snapshot get the snapshot's XML; then on the destination:
>> create the domain, then multiple domain snapshot create calls (must
>> be in topological order) with the REDEFINE flag. This patch aims to
>> make the process use fewer APIs by making it possible to grab the XML
>> for all snapshots at once on the source, then redefine from that same
>> list at the destination. In fact, although it is easy to match recent
>> changes to allow output in topological order, it is also easy to do
>> bulk redefine in arbitrary order.
>>
>> Consideration was given to adding a flag to existing APIs; but to do
>> that, there would be an asymmetry of dumping <snapshots> as a
>> sub-element of <domain> during virDomainGetXMLDesc(), vs. redefining
>> <snapshots> as a top-level element (and different than the normal
>> top-level element) of virDomainSnapshotCreateXML(). Thus, it is
>> cleaner to have two new APIs:
>>
>> virDomainGetSnapshotsXMLDesc
>> virDomainImportSnapshotsXML
> 
> These do not share a common prefix, but that makes them look nicer IMO.

In particular, they do NOT have the virDomainSnapshot prefix, which
means I don't have to special-case them in the python generator :)

> 
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> include/libvirt/libvirt-domain-snapshot.h |  16 ++-
>> src/driver-hypervisor.h                   |  13 ++-
>> src/libvirt-domain-snapshot.c             | 122 +++++++++++++++++++++-
>> src/libvirt_public.syms                   |   2 +
>> 4 files changed, 148 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt-domain-snapshot.h
>> b/include/libvirt/libvirt-domain-snapshot.h
>> index 602e5def59..7c2e001b87 100644
>> --- a/include/libvirt/libvirt-domain-snapshot.h
>> +++ b/include/libvirt/libvirt-domain-snapshot.h
>> @@ -3,7 +3,7 @@
>>  * Summary: APIs for management of domain snapshots
>>  * Description: Provides APIs for the management of domain snapshots
>>  *
>> - * Copyright (C) 2006-2014 Red Hat, Inc.
>> + * Copyright (C) 2006-2019 Red Hat, Inc.
> 
> Do we actually care about updating these?

I have an emacs hook that prompts me to auto-update on files I touch. I
know we haven't been insistent on keeping things up-to-date, but I also
don't think it hurts.

> But an include file will possibly be read by people without git access.

Yeah, especially for public-facing files, having a reliable year in
there might matter (although these days, I hope that any court
litigating a copyright claim against open source would be smart enough
to rely more on version control history than any thing in the file under
question).


>> + *
>> + * Returns a count of snapshots imported on success, or -1 on failure.
> 
> This says nothing about partial success, i.e. whether the count returned
> can be different from the number of snapshots present in the file.

Partial success is not possible; it's an all-or-none return. But I can
try to tweak that wording to make it clearer.

>> /**
>>  * virDomainSnapshotGetXMLDesc:
>>  * @snapshot: a domain snapshot object
>> @@ -254,8 +314,8 @@ virDomainSnapshotCreateXML(virDomainPtr domain,
>>  * VIR_DOMAIN_SNAPSHOT_XML_SECURE; this flag is rejected on read-only
>>  * connections.
>>  *
>> - * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case
>> of error.
>> - *         the caller must free() the returned value.
>> + * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case
>> + * of error.  The caller must free() the returned value.
> 
> Unrelated change.

Noticed while copy-and-pasting. If needed, I can split it out as a
trivial patch.

-- 
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