1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/8] more snapshot improvements [incremental backup saga]
  4. View patch

[libvirt] [PATCH 7/8] snapshot: Add ListAll filters for current snapshot

Eric Blake posted 8 patches 5 years, 4 months ago
[libvirt] [PATCH 7/8] snapshot: Add ListAll filters for current snapshot
Posted by Eric Blake 5 years, 4 months ago 
Add a new filter group VIR_DOMAIN_SNAPSHOT_LIST_CURRENT/NO_CURRENT,
which restricts the output based on whether a snapshot is the domain's
current snapshot.  This is redundant with existing API (both
virDomainHasCurrentSnapshot and virDomainSnapshotCurrent can be
emulated by a single call to virDomainListAllSnapshots, and replacing
virDomainSnapshotIsCurrent is also possible but a bit more indirect).
However, adding the new filters makes snapshots slightly more
consistent with the plans for the upcoming checkpoint API additions to
ONLY provide access to the notion of being current via list filters or
XML inspection.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/libvirt/libvirt-domain-snapshot.h |  5 +++++
 src/conf/virdomainsnapshotobjlist.h       |  7 ++++++-
 src/conf/virdomainsnapshotobjlist.c       |  4 ++++
 src/libvirt-domain-snapshot.c             | 14 ++++++++++++--
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index 90673ed0fb..02cdabafbc 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -141,6 +141,11 @@ typedef enum {
     VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur
                                                          before children in
                                                          the resulting list */
+
+    VIR_DOMAIN_SNAPSHOT_LIST_CURRENT     = (1 << 11), /* Filter to just current
+                                                         snapshot */
+    VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT  = (1 << 12), /* Filter out current
+                                                         snapshot */
 } virDomainSnapshotListFlags;

 /* Return the number of snapshots for this domain */
diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index fed8d22bc8..59a176aaef 100644
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -73,11 +73,16 @@ int virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots,
                (VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL     | \
                 VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)

+#define VIR_DOMAIN_SNAPSHOT_FILTERS_CURRENT \
+               (VIR_DOMAIN_SNAPSHOT_LIST_CURRENT      | \
+                VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT)
+
 #define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \
                (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA  | \
                 VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES    | \
                 VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS    | \
-                VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION)
+                VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION  | \
+                VIR_DOMAIN_SNAPSHOT_FILTERS_CURRENT)

 int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots,
                            virDomainMomentObjPtr from,
diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
index 99bc4bb0c5..cac0834e1f 100644
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
@@ -124,6 +124,10 @@ virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots,
           VIR_DOMAIN_MOMENT_LIST_LEAVES, },
         { VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES,
           VIR_DOMAIN_MOMENT_LIST_NO_LEAVES, },
+        { VIR_DOMAIN_SNAPSHOT_LIST_CURRENT,
+          VIR_DOMAIN_MOMENT_LIST_CURRENT, },
+        { VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT,
+          VIR_DOMAIN_MOMENT_LIST_NO_CURRENT, },
         { VIR_DOMAIN_SNAPSHOT_LIST_METADATA,
           VIR_DOMAIN_MOMENT_LIST_METADATA, },
         { VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA,
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 2687a34b96..5dd2c5390b 100644
--- a/src/libvirt-domain-snapshot.c
+++ b/src/libvirt-domain-snapshot.c
@@ -465,6 +465,12 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen,
  * whether the snapshot is stored inside the disk images or as
  * additional files.
  *
+ * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_CURRENT and
+ * VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT, to filter based on whether a
+ * snapshot is current. This filter provides the same functionality as
+ * virDomainHasCurrentSnapshot(), virDomainSnapshotCurrent(), and
+ * virDomainSnapshotIsCurrent().
+ *
  * Returns the number of domain snapshots found or -1 and sets @snaps to
  * NULL in case of error.  On success, the array stored into @snaps is
  * guaranteed to have an extra allocated element set to NULL but not included
@@ -736,7 +742,9 @@ virDomainSnapshotLookupByName(virDomainPtr domain,
  * @domain: pointer to the domain object
  * @flags: extra flags; not used yet, so callers should always pass 0
  *
- * Determine if the domain has a current snapshot.
+ * Determine if the domain has a current snapshot.  This can also be
+ * determined by using virDomainListAllSnapshots() with the
+ * VIR_DOMAIN_SNAPSHOT_LIST_CURRENT filter.
  *
  * Returns 1 if such snapshot exists, 0 if it doesn't, -1 on error.
  */
@@ -771,7 +779,9 @@ virDomainHasCurrentSnapshot(virDomainPtr domain, unsigned int flags)
  * @domain: a domain object
  * @flags: extra flags; not used yet, so callers should always pass 0
  *
- * Get the current snapshot for a domain, if any.
+ * Get the current snapshot for a domain, if any.  This can also be
+ * determined by using virDomainListAllSnapshots() with the
+ * VIR_DOMAIN_SNAPSHOT_LIST_CURRENT filter.
  *
  * virDomainSnapshotFree should be used to free the resources after the
  * snapshot object is no longer needed.
-- 
2.20.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 7/8] snapshot: Add ListAll filters for current snapshot
Posted by Peter Krempa 5 years, 4 months ago 
On Fri, Jul 05, 2019 at 23:37:34 -0500, Eric Blake wrote:
> Add a new filter group VIR_DOMAIN_SNAPSHOT_LIST_CURRENT/NO_CURRENT,
> which restricts the output based on whether a snapshot is the domain's
> current snapshot.  This is redundant with existing API (both
> virDomainHasCurrentSnapshot and virDomainSnapshotCurrent can be
> emulated by a single call to virDomainListAllSnapshots, and replacing
> virDomainSnapshotIsCurrent is also possible but a bit more indirect).
> However, adding the new filters makes snapshots slightly more
> consistent with the plans for the upcoming checkpoint API additions to
> ONLY provide access to the notion of being current via list filters or
> XML inspection.

This will be justified only if you make the same changes to expose
multiple "current" snapshots as well, since external snapshots allow for
the exact same problems which you'll get with checkpoints too.

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  include/libvirt/libvirt-domain-snapshot.h |  5 +++++
>  src/conf/virdomainsnapshotobjlist.h       |  7 ++++++-
>  src/conf/virdomainsnapshotobjlist.c       |  4 ++++
>  src/libvirt-domain-snapshot.c             | 14 ++++++++++++--
>  4 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
> index 90673ed0fb..02cdabafbc 100644
> --- a/include/libvirt/libvirt-domain-snapshot.h
> +++ b/include/libvirt/libvirt-domain-snapshot.h
> @@ -141,6 +141,11 @@ typedef enum {
>      VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur
>                                                           before children in
>                                                           the resulting list */
> +
> +    VIR_DOMAIN_SNAPSHOT_LIST_CURRENT     = (1 << 11), /* Filter to just current
> +                                                         snapshot */
> +    VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT  = (1 << 12), /* Filter out current
> +                                                         snapshot */

This will require plural form of 'snapshot'

>  } virDomainSnapshotListFlags;
> 
>  /* Return the number of snapshots for this domain */

[...]

> diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
> index 2687a34b96..5dd2c5390b 100644
> --- a/src/libvirt-domain-snapshot.c
> +++ b/src/libvirt-domain-snapshot.c
> @@ -465,6 +465,12 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen,
>   * whether the snapshot is stored inside the disk images or as
>   * additional files.
>   *
> + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_CURRENT and
> + * VIR_DOMAIN_SNAPSHOT_LIST_NO_CURRENT, to filter based on whether a
> + * snapshot is current. This filter provides the same functionality as
> + * virDomainHasCurrentSnapshot(), virDomainSnapshotCurrent(), and
> + * virDomainSnapshotIsCurrent().
> + *
>   * Returns the number of domain snapshots found or -1 and sets @snaps to
>   * NULL in case of error.  On success, the array stored into @snaps is
>   * guaranteed to have an extra allocated element set to NULL but not included
> @@ -736,7 +742,9 @@ virDomainSnapshotLookupByName(virDomainPtr domain,
>   * @domain: pointer to the domain object
>   * @flags: extra flags; not used yet, so callers should always pass 0
>   *
> - * Determine if the domain has a current snapshot.
> + * Determine if the domain has a current snapshot.  This can also be
> + * determined by using virDomainListAllSnapshots() with the
> + * VIR_DOMAIN_SNAPSHOT_LIST_CURRENT filter.

This implies that there's only one 'current' snapshot. The documentation
will need to be updated to reflect that the bulk-list API will be a
superset of the APIs.

>   *
>   * Returns 1 if such snapshot exists, 0 if it doesn't, -1 on error.
>   */

This patch makes sense if we do the same changes to support multiple
"current" snapshots similarly to what you plan with checkpoints. In that
case I'd like to see it together with the patchset making that happen.
Or at least I'd like to review the wording changes.

(The last possibility obviously is to stop thinking about "current"
snapshots and checkpoints since it's quite tricky).
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.