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

[Peter Krempa]

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 at 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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190708/0c91034b/attachment-0001.sig>