When doing REDEFINE on multiple snapshot metadata XML descriptions, we
require that a child cannot be redefined before its parent. Since
libvirt already tracks a DAG, it is more convenient if we can ensure
that virDomainListAllSnapshots() and friends have a way to return data
in an order that we can directly reuse, rather than having to
post-process the data ourselves to reconstruct the DAG.
Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a
guarantee at the time of the API call conclusion; there's always a
possible TOCTTOU race where someone redefining snapshots in between
the API results and the client actually using the list might render
the list out-of-date). Four listing APIs are directly benefitted by
the new flag; additionally, since we document that the older racy
ListNames interfaces should be sized by using the same flags on their
Num counterparts, the Num interfaces must document when they accept
(and ignore) the flag.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
include/libvirt/libvirt-domain-snapshot.h | 4 +++
src/libvirt-domain-snapshot.c | 42 ++++++++++++++++++++++-
2 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
index d9b689abbd..602e5def59 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -135,6 +135,10 @@ typedef enum {
VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL = (1 << 9), /* Filter by snapshots
that use files external
to disk images */
+
+ VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur
+ before children in
+ the resulting list */
} virDomainSnapshotListFlags;
/* Return the number of snapshots for this domain */
diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
index 11d84289f8..d133c84933 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
@@ -298,6 +298,10 @@ virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
*
* Provides the number of domain snapshots for this domain.
*
+ * This function will accept VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL in
+ * @flags only if virDomainSnapshotListNames() can honor it, although
+ * the flag has no other effect here.
+ *
* By default, this command covers all snapshots; it is also possible to
* limit things to just snapshots with no parents, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
@@ -369,6 +373,14 @@ virDomainSnapshotNum(virDomainPtr domain, unsigned int flags)
* their names in @names. The value to use for @nameslen can be determined
* by virDomainSnapshotNum() with the same @flags.
*
+ * If @flags contains VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL, and no
+ * other connection is modifying snapshots, then it is guaranteed that
+ * for any snapshot in the resulting list, then no snapshots later in
+ * the list can be reached by a sequence of
+ * virDomainSnapshotGetParent() starting from that snapshot;
+ * otherwise, the order of snapshots in the resulting list is
+ * unspecified.
+ *
* By default, this command covers all snapshots; it is also possible to
* limit things to just snapshots with no parents, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
@@ -457,6 +469,14 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen,
* an array to store those objects. This API solves the race inherent in
* virDomainSnapshotListNames().
*
+ * If @flags contains VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL and @snaps
+ * is non-NULL, and no other connection is modifying snapshots, then
+ * it is guaranteed that for any snapshot in the resulting list, no
+ * snapshots later in the list can be reached by a sequence of
+ * virDomainSnapshotGetParent() starting from that snapshot;
+ * otherwise, the order of snapshots in the resulting list is
+ * unspecified.
+ *
* By default, this command covers all snapshots; it is also possible to
* limit things to just snapshots with no parents, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
@@ -533,6 +553,10 @@ virDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps,
*
* Provides the number of child snapshots for this domain snapshot.
*
+ * This function will accept VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL in
+ * @flags only if virDomainSnapshotListChildrenNames() can honor it,
+ * although the flag has no other effect here.
+ *
* By default, this command covers only direct children; it is also possible
* to expand things to cover all descendants, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in
@@ -605,6 +629,14 @@ virDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags)
* @nameslen can be determined by virDomainSnapshotNumChildren() with
* the same @flags.
*
+ * If @flags lacks VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS or contains
+ * VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL, and no other connection is
+ * modifying snapshots, then it is guaranteed that for any snapshot in
+ * the resulting list, no snapshots later in the list can be reached
+ * by a sequence of virDomainSnapshotGetParent() starting from that
+ * snapshot; otherwise, the order of snapshots in the resulting list
+ * is unspecified.
+ *
* By default, this command covers only direct children; it is also possible
* to expand things to cover all descendants, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in
@@ -697,6 +729,14 @@ virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
* snapshot, and allocate an array to store those objects. This API solves
* the race inherent in virDomainSnapshotListChildrenNames().
*
+ * If @flags lacks VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS or contains
+ * VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL, @snaps is non-NULL, and no
+ * other connection is modifying snapshots, then it is guaranteed that
+ * for any snapshot in the resulting list, no snapshots later in the
+ * list can be reached by a sequence of virDomainSnapshotGetParent()
+ * starting from that snapshot; otherwise, the order of snapshots in
+ * the resulting list is unspecified.
+ *
* By default, this command covers only direct children; it is also possible
* to expand things to cover all descendants, when @flags includes
* VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 08, 2019 at 12:05:08AM -0600, Eric Blake wrote:
>When doing REDEFINE on multiple snapshot metadata XML descriptions, we
>require that a child cannot be redefined before its parent. Since
>libvirt already tracks a DAG, it is more convenient if we can ensure
>that virDomainListAllSnapshots() and friends have a way to return data
>in an order that we can directly reuse, rather than having to
>post-process the data ourselves to reconstruct the DAG.
>
>Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a
>guarantee at the time of the API call conclusion; there's always a
>possible TOCTTOU race where someone redefining snapshots in between
>the API results and the client actually using the list might render
>the list out-of-date). Four listing APIs are directly benefitted by
>the new flag; additionally, since we document that the older racy
>ListNames interfaces should be sized by using the same flags on their
>Num counterparts, the Num interfaces must document when they accept
>(and ignore) the flag.
>
I'd rather deal with that by not introducing the new flag to the old
APIs. The ListAll APIs were introduced back in 2012 and are even older
than the minimum QEMU version we support.
>Signed-off-by: Eric Blake <eblake@redhat.com>
>---
> include/libvirt/libvirt-domain-snapshot.h | 4 +++
> src/libvirt-domain-snapshot.c | 42 ++++++++++++++++++++++-
> 2 files changed, 45 insertions(+), 1 deletion(-)
>
>diff --git a/include/libvirt/libvirt-domain-snapshot.h b/include/libvirt/libvirt-domain-snapshot.h
>index d9b689abbd..602e5def59 100644
>--- a/include/libvirt/libvirt-domain-snapshot.h
>+++ b/include/libvirt/libvirt-domain-snapshot.h
>@@ -135,6 +135,10 @@ typedef enum {
> VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL = (1 << 9), /* Filter by snapshots
> that use files external
> to disk images */
>+
>+ VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL = (1 << 10), /* Ensure parents occur
>+ before children in
>+ the resulting list */
> } virDomainSnapshotListFlags;
>
> /* Return the number of snapshots for this domain */
>diff --git a/src/libvirt-domain-snapshot.c b/src/libvirt-domain-snapshot.c
>index 11d84289f8..d133c84933 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
So only these two hunks would be needed:
>@@ -457,6 +469,14 @@ virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen,
> * an array to store those objects. This API solves the race inherent in
> * virDomainSnapshotListNames().
> *
>+ * If @flags contains VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL and @snaps
>+ * is non-NULL, and no other connection is modifying snapshots, then
>+ * it is guaranteed that for any snapshot in the resulting list, no
>+ * snapshots later in the list can be reached by a sequence of
>+ * virDomainSnapshotGetParent() starting from that snapshot;
>+ * otherwise, the order of snapshots in the resulting list is
>+ * unspecified.
>+ *
> * By default, this command covers all snapshots; it is also possible to
> * limit things to just snapshots with no parents, when @flags includes
> * VIR_DOMAIN_SNAPSHOT_LIST_ROOTS. Additional filters are provided in
>@@ -697,6 +729,14 @@ virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
> * snapshot, and allocate an array to store those objects. This API solves
> * the race inherent in virDomainSnapshotListChildrenNames().
> *
>+ * If @flags lacks VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS or contains
>+ * VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL, @snaps is non-NULL, and no
>+ * other connection is modifying snapshots, then it is guaranteed that
>+ * for any snapshot in the resulting list, no snapshots later in the
>+ * list can be reached by a sequence of virDomainSnapshotGetParent()
>+ * starting from that snapshot; otherwise, the order of snapshots in
>+ * the resulting list is unspecified.
>+ *
> * By default, this command covers only direct children; it is also possible
> * to expand things to cover all descendants, when @flags includes
> * VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS. Also, some filters are provided in
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
On 3/12/19 11:03 AM, Ján Tomko wrote: > On Fri, Mar 08, 2019 at 12:05:08AM -0600, Eric Blake wrote: >> When doing REDEFINE on multiple snapshot metadata XML descriptions, we >> require that a child cannot be redefined before its parent. Since >> libvirt already tracks a DAG, it is more convenient if we can ensure >> that virDomainListAllSnapshots() and friends have a way to return data >> in an order that we can directly reuse, rather than having to >> post-process the data ourselves to reconstruct the DAG. >> >> Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a >> guarantee at the time of the API call conclusion; there's always a >> possible TOCTTOU race where someone redefining snapshots in between >> the API results and the client actually using the list might render >> the list out-of-date). Four listing APIs are directly benefitted by >> the new flag; additionally, since we document that the older racy >> ListNames interfaces should be sized by using the same flags on their >> Num counterparts, the Num interfaces must document when they accept >> (and ignore) the flag. >> > > I'd rather deal with that by not introducing the new flag to the old > APIs. The ListAll APIs were introduced back in 2012 and are even older > than the minimum QEMU version we support. It's easy enough (in later patches) to NOT accept the new flag in the virCheckFlags() macro for the implementation of those functions. On the other hand, it's trivial to support. But if you don't like new flags to old APIs, even though the old and new APIs share a common enum, I can make that change. > > Reviewed-by: Ján Tomko <jtomko@redhat.com> > > Jano -- 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
On Tue, Mar 12, 2019 at 05:03:32PM +0100, Ján Tomko wrote: > On Fri, Mar 08, 2019 at 12:05:08AM -0600, Eric Blake wrote: > > When doing REDEFINE on multiple snapshot metadata XML descriptions, we > > require that a child cannot be redefined before its parent. Since > > libvirt already tracks a DAG, it is more convenient if we can ensure > > that virDomainListAllSnapshots() and friends have a way to return data > > in an order that we can directly reuse, rather than having to > > post-process the data ourselves to reconstruct the DAG. > > > > Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a > > guarantee at the time of the API call conclusion; there's always a > > possible TOCTTOU race where someone redefining snapshots in between > > the API results and the client actually using the list might render > > the list out-of-date). Four listing APIs are directly benefitted by > > the new flag; additionally, since we document that the older racy > > ListNames interfaces should be sized by using the same flags on their > > Num counterparts, the Num interfaces must document when they accept > > (and ignore) the flag. > > > > I'd rather deal with that by not introducing the new flag to the old > APIs. The ListAll APIs were introduced back in 2012 and are even older > than the minimum QEMU version we support. I don't much like that idea. The existing virDomainSnapshotListFlags constants apply to all the APIs. Special casing the new enum constant so that it only applies to some of the APIs is needlessly creating an inconsistency for no obvious benefit. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Mar 08, 2019 at 12:05:08AM -0600, Eric Blake wrote: > When doing REDEFINE on multiple snapshot metadata XML descriptions, we > require that a child cannot be redefined before its parent. Since > libvirt already tracks a DAG, it is more convenient if we can ensure > that virDomainListAllSnapshots() and friends have a way to return data > in an order that we can directly reuse, rather than having to > post-process the data ourselves to reconstruct the DAG. > > Add VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL as our new guarantee (well, a > guarantee at the time of the API call conclusion; there's always a > possible TOCTTOU race where someone redefining snapshots in between > the API results and the client actually using the list might render > the list out-of-date). Four listing APIs are directly benefitted by > the new flag; additionally, since we document that the older racy > ListNames interfaces should be sized by using the same flags on their > Num counterparts, the Num interfaces must document when they accept > (and ignore) the flag. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > include/libvirt/libvirt-domain-snapshot.h | 4 +++ > src/libvirt-domain-snapshot.c | 42 ++++++++++++++++++++++- > 2 files changed, 45 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.