It turns out that one-bit filtering makes it hard to select the inverse set, so it is easier to provide filtering groups. For back-compat, omitting all bits within a group means the group is not used for filtering, and by definition of a group (each snapshot matches exactly one bit within the group, and the set of bits in the group covers all snapshots), selecting all bits also makes the group useless. Unfortunately, virDomainSnapshotListChildren defined the bit VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS as an expansion rather than a filter, so we cannot make it part of a filter group, so that bit (and its counterpart VIR_DOMAIN_SNAPSHOT_LIST_ROOTS for virDomainSnapshotList) remains a single control bit. * include/libvirt/libvirt.h.in (virDomainSnapshotListFlags): Add a couple more flags. * src/libvirt.c (virDomainSnapshotNum) (virDomainSnapshotNumChildren): Document them. (virDomainSnapshotListNames, virDomainSnapshotListChildrenNames): Likewise, and add thread-safety caveats. * src/conf/domain_conf.c (virDomainSnapshotObjListPrepFlags): New helper function. (virDomainSnapshotObjListCopyNames) (virDomainSnapshotObjListCount): Support the new flags. --- include/libvirt/libvirt.h.in | 22 +++++-- src/conf/domain_conf.c | 53 +++++++++++++--- src/libvirt.c | 138 ++++++++++++++++++++++++++++++------------ 3 files changed, 161 insertions(+), 52 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f2ee1d6..97cf46d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3352,10 +3352,16 @@ virDomainSnapshotPtr virDomainSnapshotCreateXML(virDomainPtr domain, char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, unsigned int flags); -/* Flags valid for virDomainSnapshotNum(), +/** + * virDomainSnapshotListFlags: + * + * Flags valid for virDomainSnapshotNum(), * virDomainSnapshotListNames(), virDomainSnapshotNumChildren(), and * virDomainSnapshotListChildrenNames(). Note that the interpretation - * of flag (1<<0) depends on which function it is passed to. */ + * of flag (1<<0) depends on which function it is passed to; but serves + * to toggle the per-call default of whether the listing is shallow or + * recursive. Remaining bits come in groups; if all bits from a group are + * 0, then that group is not used to filter results. */ typedef enum { VIR_DOMAIN_SNAPSHOT_LIST_ROOTS = (1 << 0), /* Filter by snapshots with no parents, when @@ -3363,10 +3369,18 @@ typedef enum { VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS = (1 << 0), /* List all descendants, not just children, when listing a snapshot */ - VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots - which have metadata */ + + /* For historical reasons, groups do not use contiguous bits. */ + VIR_DOMAIN_SNAPSHOT_LIST_LEAVES = (1 << 2), /* Filter by snapshots with no children */ + VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES = (1 << 3), /* Filter by snapshots + that have children */ + + VIR_DOMAIN_SNAPSHOT_LIST_METADATA = (1 << 1), /* Filter by snapshots + which have metadata */ + VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA = (1 << 4), /* Filter by snapshots + with no metadata */ } virDomainSnapshotListFlags; /* Return the number of snapshots for this domain */ diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 221e1d0..dab381d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14239,6 +14239,37 @@ virDomainSnapshotObjListDeinit(virDomainSnapshotObjListPtr snapshots) virHashFree(snapshots->objs); } +/* Sanitize the filtering FLAGS and set *DST before iterating over + * snapshots; return 0 if filtering might succeed, -1 if the choice of + * flags guarantees no results. */ +static int +virDomainSnapshotObjListPrepFlags(unsigned int *dst, unsigned int flags) +{ + /* We filter LIST_ROOT/LIST_DESCENDANTS separately in the caller; + * mask that bit out, to make visitor code simpler. */ + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + + /* If this common code is being used, we assume that all snapshots + * have metadata, and thus can handle METADATA up front as an + * all-or-none filter. XXX This might not always be true, if we + * add the ability to track qcow2 internal snapshots without the + * use of metadata. */ + if ((flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) && + !(flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA)) + return -1; + flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_METADATA | + VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA); + + /* For ease of coding the visitor, it is easier to zero the LEAVES + * group if both bits are set. */ + if ((flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && + (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES)) + flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_LEAVES | + VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES); + *dst = flags; + return 0; +} + struct virDomainSnapshotNameData { int oom; int numnames; @@ -14256,10 +14287,11 @@ static void virDomainSnapshotObjListCopyNames(void *payload, if (data->oom) return; - /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, - * LIST_METADATA is a no-op if we get this far. */ + /* See virDomainSnapshotObjListPrepFlags; flags was sanitized. */ if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) return; + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) && !obj->nchildren) + return; if (data->numnames < data->maxnames) { if (!(data->names[data->numnames] = strdup(obj->def->name))) @@ -14276,7 +14308,8 @@ int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; int i; - data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) + return 0; if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, @@ -14308,7 +14341,8 @@ int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; int i; - data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) + return 0; if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) virDomainSnapshotForEachDescendant(snapshot, @@ -14343,10 +14377,11 @@ static void virDomainSnapshotObjListCount(void *payload, virDomainSnapshotObjPtr obj = payload; struct virDomainSnapshotNumData *data = opaque; - /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, - * LIST_METADATA is a no-op if we get this far. */ + /* See virDomainSnapshotObjListPrepFlags; flags was sanitized. */ if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren) return; + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) && !obj->nchildren) + return; data->count++; } @@ -14355,7 +14390,8 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, { struct virDomainSnapshotNumData data = { 0, 0 }; - data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) + return 0; if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data); @@ -14378,7 +14414,8 @@ virDomainSnapshotObjListNumFrom(virDomainSnapshotObjPtr snapshot, { struct virDomainSnapshotNumData data = { 0, 0 }; - data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) + return 0; if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) virDomainSnapshotForEachDescendant(snapshot, diff --git a/src/libvirt.c b/src/libvirt.c index 16afd58..fd08b13 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16939,16 +16939,27 @@ error: * * Provides the number of domain snapshots for this domain. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is - * filtered to the number of snapshots that have no parents. Likewise, - * if @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. Both flags - * can be used together to find unrelated snapshots. - * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * 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 + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. */ @@ -16993,18 +17004,36 @@ error: * of the array. The value to use for @nameslen can be determined by * virDomainSnapshotNum() with the same @flags. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, then the result is - * filtered to the number of snapshots that have no parents. Likewise, - * if @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. Both flags - * can be used together to find unrelated snapshots. - * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * 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 + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. + * Note that this command is inherently racy: another connection can + * define a new snapshot between a call to virDomainSnapshotNum() and + * this call. You are only guaranteed that all currently defined + * snapshots were listed if the return is less than @nameslen. Likewise, + * you should be prepared for virDomainSnapshotLookupByName() to fail when + * converting a name from this call into a snapshot object, if another + * connection deletes the snapshot in the meantime. */ int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, @@ -17049,16 +17078,27 @@ error: * * Provides the number of child snapshots for this domain snapshot. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, then the result - * includes all descendants, otherwise it is limited to direct children. - * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. - * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * 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 + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. */ @@ -17104,18 +17144,36 @@ error: * freeing each member of the array. The value to use for @nameslen can * be determined by virDomainSnapshotNumChildren() with the same @flags. * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, then the result - * includes all descendants, otherwise it is limited to direct children. - * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, then the result is - * filtered to the number of snapshots that have no children. - * - * If @flags includes VIR_DOMAIN_SNAPSHOT_LIST_METADATA, then the result is - * the number of snapshots that also include metadata that would prevent - * the removal of the last reference to a domain; this value will either - * be 0 or the same value as if the flag were not given. + * 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 + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. * * Returns the number of domain snapshots found or -1 in case of error. + * Note that this command is inherently racy: another connection can + * define a new snapshot between a call to virDomainSnapshotNumChildren() + * and this call. You are only guaranteed that all currently defined + * snapshots were listed if the return is less than @nameslen. Likewise, + * you should be prepared for virDomainSnapshotLookupByName() to fail when + * converting a name from this call into a snapshot object, if another + * connection deletes the snapshot in the meantime. */ int virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, -- 1.7.10.2

Previously, to get the name of all snapshots with children, it was necessary to get the name of all snapshots and then remove the name of leaf snapshots. This is racy, and somewhat inefficient compared to planned API additions. We can emulate --no-metadata on 0.9.5-0.9.12, but for now, there is no emulation of --no-leaves. * tools/virsh.c (cmdSnapshotList): Add new options --no-leaves and --no-metadata. (vshSnapshotList): Emulate where possible. * tools/virsh.pod (snapshot-list): Document them. --- tools/virsh.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 14 ++++++++++---- 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 1228508..936b9fe 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16762,6 +16762,30 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, /* 0.9.13 will be adding a new listing API. */ + /* Assume that if we got this far, then the --no-leaves and + * --no-metadata flags were not supported. Disable groups that + * have no impact. */ + /* XXX should we emulate --no-leaves? */ + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES && + flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) + flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES | + VIR_DOMAIN_SNAPSHOT_LIST_LEAVES); + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA && + flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA) + flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA | + VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA) { + /* We can emulate --no-metadata if --metadata was supported, + * since it was an all-or-none attribute on old servers. */ + count = virDomainSnapshotNum(dom, + VIR_DOMAIN_SNAPSHOT_LIST_METADATA); + if (count < 0) + goto cleanup; + if (count > 0) + return snaplist; + flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_METADATA; + } + /* This is the interface available in 0.9.12 and earlier, * including fallbacks to 0.9.4 and earlier. */ if (from) { @@ -16986,8 +17010,12 @@ static const vshCmdOptDef opts_snapshot_list[] = { {"parent", VSH_OT_BOOL, 0, N_("add a column showing parent snapshot")}, {"roots", VSH_OT_BOOL, 0, N_("list only snapshots without parents")}, {"leaves", VSH_OT_BOOL, 0, N_("list only snapshots without children")}, + {"no-leaves", VSH_OT_BOOL, 0, + N_("list only snapshots that are not leaves (with children)")}, {"metadata", VSH_OT_BOOL, 0, N_("list only snapshots that have metadata that would prevent undefine")}, + {"no-metadata", VSH_OT_BOOL, 0, + N_("list only snapshots that have no metadata managed by libvirt")}, {"tree", VSH_OT_BOOL, 0, N_("list snapshots in a tree")}, {"from", VSH_OT_DATA, 0, N_("limit list to children of given snapshot")}, {"current", VSH_OT_BOOL, 0, @@ -17016,9 +17044,9 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) struct tm time_info; bool tree = vshCommandOptBool(cmd, "tree"); bool leaves = vshCommandOptBool(cmd, "leaves"); + bool no_leaves = vshCommandOptBool(cmd, "no-leaves"); const char *from = NULL; virDomainSnapshotPtr start = NULL; - bool descendants = false; vshSnapshotListPtr snaplist = NULL; if (!vshConnectionUsability(ctl, ctl->conn)) @@ -17066,15 +17094,28 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } flags |= VIR_DOMAIN_SNAPSHOT_LIST_LEAVES; } + if (no_leaves) { + if (tree) { + vshError(ctl, "%s", + _("--no-leaves and --tree are mutually exclusive")); + goto cleanup; + } + flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES; + } if (vshCommandOptBool(cmd, "metadata")) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; } - - if (from) { - descendants = vshCommandOptBool(cmd, "descendants"); - if (descendants) - flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; + if (vshCommandOptBool(cmd, "no-metadata")) { + flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA; + } + if (vshCommandOptBool(cmd, "descendants")) { + if (!from) { + vshError(ctl, "%s", + _("--descendants requires either --from or --current")); + goto cleanup; + } + flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; } if ((snaplist = vshSnapshotListCollect(ctl, dom, start, flags, diff --git a/tools/virsh.pod b/tools/virsh.pod index 6553825..efdf41f 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2511,7 +2511,7 @@ with I<--current>. =item B<snapshot-list> I<domain> [{I<--parent> | I<--roots> | I<--tree>}] [{[I<--from>] B<snapshot> | I<--current>} [I<--descendants>]] -[I<--metadata>] [I<--leaves>] +[I<--metadata>] [I<--no-metadata>] [I<--leaves>] [I<--no-leaves>] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -2531,13 +2531,19 @@ use of I<--descendants> is implied. This option is not compatible with I<--roots>. If I<--leaves> is specified, the list will be filtered to just -snapshots that have no children. This option is not compatible -with I<--tree>. +snapshots that have no children. Likewise, if I<--no-leaves> is +specified, the list will be filtered to just snapshots with +children. (Note that omitting both options does no filtering, +while providing both options will either produce the same list +or error out depending on whether the server recognizes the flags). +These options are not compatible with I<--tree>. If I<--metadata> is specified, the list will be filtered to just snapshots that involve libvirt metadata, and thus would prevent B<undefine> of a persistent domain, or be lost on B<destroy> of -a transient domain. +a transient domain. Likewise, if I<--no-metadata> is specified, +the list will be filtered to just snapshots that exist without +the need for libvirt metadata. =item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] -- 1.7.10.2

There was an inherent race between virDomainSnapshotNum() and virDomainSnapshotListNames(), where an additional snapshot could be created in the meantime, or where a snapshot could be deleted before converting the name back to a virDomainSnapshotPtr. It was also an awkward name: the function operates on domains, not domain snapshots. virDomainSnapshotListChildrenNames() suffered from the same inherent race, although its naming was nicer. This patch makes things nicer by grabbing a snapshot list atomically, in the format most useful to the user. * include/libvirt/libvirt.h.in (virDomainListAllSnapshots) (virDomainSnapshotListAllChildren): New declarations. * src/libvirt.c (virDomainListAllSnapshots) (virDomainSnapshotListAllChildren): New functions. * src/libvirt_public.syms (LIBVIRT_0.9.13): Export them. * src/driver.h (virDrvDomainListAllSnapshots) (virDrvDomainSnapshotListAllChildren): New callbacks. * python/generator.py (skip_function): Prepare for later hand-written versions. --- include/libvirt/libvirt.h.in | 13 +++- python/generator.py | 2 + src/driver.h | 12 ++++ src/libvirt.c | 143 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 2 + 5 files changed, 171 insertions(+), 1 deletion(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 97cf46d..b47f1bc 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3357,7 +3357,8 @@ char *virDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, * * Flags valid for virDomainSnapshotNum(), * virDomainSnapshotListNames(), virDomainSnapshotNumChildren(), and - * virDomainSnapshotListChildrenNames(). Note that the interpretation + * virDomainSnapshotListChildrenNames(), virDomainListAllSnapshots(), + * and virDomainSnapshotListAllChildren(). Note that the interpretation * of flag (1<<0) depends on which function it is passed to; but serves * to toggle the per-call default of whether the listing is shallow or * recursive. Remaining bits come in groups; if all bits from a group are @@ -3390,6 +3391,11 @@ int virDomainSnapshotNum(virDomainPtr domain, unsigned int flags); int virDomainSnapshotListNames(virDomainPtr domain, char **names, int nameslen, unsigned int flags); +/* Get all snapshot objects for this domain */ +int virDomainListAllSnapshots(virDomainPtr domain, + virDomainSnapshotPtr **snaps, + unsigned int flags); + /* Return the number of child snapshots for this snapshot */ int virDomainSnapshotNumChildren(virDomainSnapshotPtr snapshot, unsigned int flags); @@ -3399,6 +3405,11 @@ int virDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot, char **names, int nameslen, unsigned int flags); +/* Get all snapshot object children for this snapshot */ +int virDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags); + /* Get a handle to a named snapshot */ virDomainSnapshotPtr virDomainSnapshotLookupByName(virDomainPtr domain, const char *name, diff --git a/python/generator.py b/python/generator.py index 9530867..67ee0db 100755 --- a/python/generator.py +++ b/python/generator.py @@ -453,6 +453,8 @@ skip_function = ( 'virConnectDomainEventDeregisterAny', # overridden in virConnect.py 'virSaveLastError', # We have our own python error wrapper 'virFreeError', # Only needed if we use virSaveLastError + 'virDomainListAllSnapshots', # overridden in virDomain.py + 'virDomainSnapshotListAllChildren', # overridden in virDomainSnapshot.py 'virStreamRecvAll', # Pure python libvirt-override-virStream.py 'virStreamSendAll', # Pure python libvirt-override-virStream.py diff --git a/src/driver.h b/src/driver.h index 94cc851..e307ff7 100644 --- a/src/driver.h +++ b/src/driver.h @@ -624,6 +624,11 @@ typedef int unsigned int flags); typedef int + (*virDrvDomainListAllSnapshots)(virDomainPtr domain, + virDomainSnapshotPtr **snaps, + unsigned int flags); + +typedef int (*virDrvDomainSnapshotNumChildren)(virDomainSnapshotPtr snapshot, unsigned int flags); @@ -633,6 +638,11 @@ typedef int int nameslen, unsigned int flags); +typedef int + (*virDrvDomainSnapshotListAllChildren)(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags); + typedef virDomainSnapshotPtr (*virDrvDomainSnapshotLookupByName)(virDomainPtr domain, const char *name, @@ -988,8 +998,10 @@ struct _virDriver { virDrvDomainSnapshotGetXMLDesc domainSnapshotGetXMLDesc; virDrvDomainSnapshotNum domainSnapshotNum; virDrvDomainSnapshotListNames domainSnapshotListNames; + virDrvDomainListAllSnapshots domainListAllSnapshots; virDrvDomainSnapshotNumChildren domainSnapshotNumChildren; virDrvDomainSnapshotListChildrenNames domainSnapshotListChildrenNames; + virDrvDomainSnapshotListAllChildren domainSnapshotListAllChildren; virDrvDomainSnapshotLookupByName domainSnapshotLookupByName; virDrvDomainHasCurrentSnapshot domainHasCurrentSnapshot; virDrvDomainSnapshotGetParent domainSnapshotGetParent; diff --git a/src/libvirt.c b/src/libvirt.c index fd08b13..b528623 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17072,6 +17072,76 @@ error: } /** + * virDomainListAllSnapshots: + * @domain: a domain object + * @snaps: pointer to variable to store the array containing snapshot objects, + * or NULL if the list is not required (just returns number of + * snapshots) + * @flags: bitwise-OR of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots for the given domain, and allocate + * an array to store those objects. Caller is responsible for calling + * virDomainSnapshotFree() on each member of the array, then free() on the + * array itself. + * + * 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 + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. + * + * Returns the number of domain snapshots found or -1 in case of error. + * On success, the array stored into @snaps is guaranteed to have an + * extra allocated element set to NULL, to make iteration easier. + */ +int +virDomainListAllSnapshots(virDomainPtr domain, virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "snaps=%p, flags=%x", snaps, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = domain->conn; + + if (conn->driver->domainListAllSnapshots) { + int ret = conn->driver->domainListAllSnapshots(domain, snaps, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainSnapshotNumChildren: * @snapshot: a domain snapshot object * @flags: bitwise-OR of supported virDomainSnapshotListFlags @@ -17216,6 +17286,79 @@ error: } /** + * virDomainSnapshotListAllChildren: + * @snapshot: a domain snapshot object + * @snaps: pointer to variable to store the array containing snapshot objects, + * or NULL if the list is not required (just returns number of + * snapshots) + * @flags: bitwise-OR of supported virDomainSnapshotListFlags + * + * Collect the list of domain snapshots that are children of the given + * snapshot, and allocate an array to store those objects. Caller is + * responsible for calling virDomainSnapshotFree() on each member of the + * array, then free() on the array itself. + * + * 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 + * groups, where each group contains bits that describe mutually exclusive + * attributes of a snapshot, and where all bits within a group describe + * all possible snapshots. Some hypervisors might reject explicit bits + * from a group where the hypervisor cannot make a distinction. For a + * group supported by a given hypervisor, the behavior when no bits of a + * group are set is identical to the behavior when all bits in that group + * are set. When setting bits from more than one group, it is possible to + * select an impossible combination, in that case a hypervisor may return + * either 0 or an error. + * + * The first group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_LEAVES and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, to filter based on snapshots that + * have no further children (a leaf snapshot). + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_METADATA and + * VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, for filtering snapshots based on + * whether they have metadata that would prevent the removal of the last + * reference to a domain. + * + * Returns the number of domain snapshots found or -1 in case of error. + * On success, the array stored into @snaps is guaranteed to have an + * extra allocated element set to NULL, to make iteration easier. + */ +int +virDomainSnapshotListAllChildren(virDomainSnapshotPtr snapshot, + virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DEBUG("snapshot=%p, snaps=%p, flags=%x", snapshot, snaps, flags); + + virResetLastError(); + + if (!VIR_IS_DOMAIN_SNAPSHOT(snapshot)) { + virLibDomainSnapshotError(VIR_ERR_INVALID_DOMAIN_SNAPSHOT, + __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + conn = snapshot->domain->conn; + + if (conn->driver->domainSnapshotListAllChildren) { + int ret = conn->driver->domainSnapshotListAllChildren(snapshot, snaps, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); +error: + virDispatchError(conn); + return -1; +} + +/** * virDomainSnapshotLookupByName: * @domain: a domain object * @name: name for the domain snapshot diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 96897de..f3f8c68 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -536,8 +536,10 @@ LIBVIRT_0.9.11 { LIBVIRT_0.9.13 { global: + virDomainListAllSnapshots; virDomainSnapshotHasMetadata; virDomainSnapshotIsCurrent; + virDomainSnapshotListAllChildren; virDomainSnapshotRef; } LIBVIRT_0.9.11; -- 1.7.10.2

This adds support for the new virDomainListAllSnapshots (a domain function) and virDomainSnapshotListAllChildren (a snapshot function) to the libvirt-python bindings. The implementation is done manually as the generator does not support wrapping lists of C pointers into python objects. * python/libvirt-override.c (libvirt_virDomainListAllSnapshots) (libvirt_virDomainSnapshotListAllChildren): New functions. * python/libvirt-override-api.xml: Document them. * python/libvirt-override-virDomain.py (listAllSnapshots): New file. * python/libvirt-override-virDomainSnapshot.py (listAllChildren): Likewise. * python/Makefile.am (CLASSES_EXTRA): Ship them. --- python/Makefile.am | 2 + python/libvirt-override-api.xml | 16 ++++- python/libvirt-override-virDomain.py | 11 ++++ python/libvirt-override-virDomainSnapshot.py | 11 ++++ python/libvirt-override.c | 90 ++++++++++++++++++++++++++ 5 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 python/libvirt-override-virDomain.py create mode 100644 python/libvirt-override-virDomainSnapshot.py diff --git a/python/Makefile.am b/python/Makefile.am index 02b59eb..97f21c3 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -24,6 +24,8 @@ DOCS = ${srcdir}/TODO CLASSES_EXTRA = \ libvirt-override-virConnect.py \ + libvirt-override-virDomain.py \ + libvirt-override-virDomainSnapshot.py \ libvirt-override-virStream.py EXTRA_DIST = \ diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index 0bafd21..106b882 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -400,17 +400,29 @@ <arg name='flags' type='unsigned int' info='fine-tuning flags, currently unused, pass 0.'/> </function> <function name='virDomainSnapshotListNames' file='python'> - <info>collect the list of snapshots for the given domain</info> + <info>collect the list of snapshot names for the given domain</info> <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> <arg name='flags' type='unsigned int' info='flags'/> <return type='str *' info='the list of Names or None in case of error'/> </function> + <function name='virDomainListAllSnapshots' file='python'> + <info>returns the list of snapshots for the given domain</info> + <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> + <arg name='flags' type='unsigned int' info='flags'/> + <return type='snapshot *' info='the list of snapshots or None in case of error'/> + </function> <function name='virDomainSnapshotListChildrenNames' file='python'> - <info>collect the list of child snapshots for the given snapshot</info> + <info>collect the list of child snapshot names for the given snapshot</info> <arg name='snapshot' type='virDomainSnapshotPtr' info='pointer to the snapshot'/> <arg name='flags' type='unsigned int' info='flags'/> <return type='str *' info='the list of Names or None in case of error'/> </function> + <function name='virDomainSnapshotListAllChildren' file='python'> + <info>returns the list of child snapshots for the given snapshot</info> + <arg name='snapshot' type='virDomainSnapshotPtr' info='pointer to the snapshot'/> + <arg name='flags' type='unsigned int' info='flags'/> + <return type='snapshot *' info='the list of snapshots or None in case of error'/> + </function> <function name='virDomainRevertToSnapshot' file='python'> <info>revert the domain to the given snapshot</info> <arg name='dom' type='virDomainPtr' info='dummy domain pointer'/> diff --git a/python/libvirt-override-virDomain.py b/python/libvirt-override-virDomain.py new file mode 100644 index 0000000..ccc4d5f --- /dev/null +++ b/python/libvirt-override-virDomain.py @@ -0,0 +1,11 @@ + def listAllSnapshots(self, flags): + """List all snapshots and returns a list of snapshot objects""" + ret = libvirtmod.virDomainListAllSnapshots(self._o, flags) + if ret is None: + raise libvirtError("virDomainListAllSnapshots() failed", conn=self) + + retlist = list() + for snapptr in ret: + retlist.append(virDomainSnapshot(self, _obj=snapptr)) + + return retlist diff --git a/python/libvirt-override-virDomainSnapshot.py b/python/libvirt-override-virDomainSnapshot.py new file mode 100644 index 0000000..3da7bfd --- /dev/null +++ b/python/libvirt-override-virDomainSnapshot.py @@ -0,0 +1,11 @@ + def listAllChildren(self, flags): + """List all child snapshots and returns a list of snapshot objects""" + ret = libvirtmod.virDomainSnapshotListAllChildren(self._o, flags) + if ret is None: + raise libvirtError("virDomainSnapshotListAllChildren() failed", conn=self) + + retlist = list() + for snapptr in ret: + retlist.append(virDomainSnapshot(self, _obj=snapptr)) + + return retlist diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 130e702..991d87b 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -2074,6 +2074,50 @@ libvirt_virDomainSnapshotListNames(PyObject *self ATTRIBUTE_UNUSED, } static PyObject * +libvirt_virDomainListAllSnapshots(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = NULL; + virDomainSnapshotPtr *snaps = NULL; + int c_retval, i; + virDomainPtr dom; + PyObject *pyobj_dom; + unsigned int flags; + PyObject *pyobj_snap; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainListAllSnapshots", + &pyobj_dom, &flags)) + return NULL; + dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainListAllSnapshots(dom, &snaps, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) { + Py_XDECREF(pyobj_snap); + Py_DECREF(py_retval); + goto cleanup; + } + snaps[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + return py_retval; +} + +static PyObject * libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { PyObject *py_retval; @@ -2118,6 +2162,50 @@ libvirt_virDomainSnapshotListChildrenNames(PyObject *self ATTRIBUTE_UNUSED, } static PyObject * +libvirt_virDomainSnapshotListAllChildren(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + PyObject *py_retval = NULL; + virDomainSnapshotPtr *snaps = NULL; + int c_retval, i; + virDomainSnapshotPtr parent; + PyObject *pyobj_parent; + unsigned int flags; + PyObject *pyobj_snap; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainSnapshotListAllChildren", + &pyobj_parent, &flags)) + return NULL; + parent = (virDomainSnapshotPtr) PyvirDomainSnapshot_Get(pyobj_parent); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_retval = virDomainSnapshotListAllChildren(parent, &snaps, flags); + LIBVIRT_END_ALLOW_THREADS; + if (c_retval < 0) + return VIR_PY_NONE; + + if (!(py_retval = PyList_New(c_retval))) + goto cleanup; + + for (i = 0; i < c_retval; i++) { + if ((pyobj_snap = libvirt_virDomainSnapshotPtrWrap(snaps[i])) == NULL || + PyList_SetItem(py_retval, i, pyobj_snap) < 0) { + Py_XDECREF(pyobj_snap); + Py_DECREF(py_retval); + goto cleanup; + } + snaps[i] = NULL; + } + +cleanup: + for (i = 0; i < c_retval; i++) + if (snaps[i]) + virDomainSnapshotFree(snaps[i]); + VIR_FREE(snaps); + return py_retval; +} + +static PyObject * libvirt_virDomainRevertToSnapshot(PyObject *self ATTRIBUTE_UNUSED, PyObject *args) { int c_retval; @@ -5699,7 +5787,9 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virConnectBaselineCPU", libvirt_virConnectBaselineCPU, METH_VARARGS, NULL}, {(char *) "virDomainGetJobInfo", libvirt_virDomainGetJobInfo, METH_VARARGS, NULL}, {(char *) "virDomainSnapshotListNames", libvirt_virDomainSnapshotListNames, METH_VARARGS, NULL}, + {(char *) "virDomainListAllSnapshots", libvirt_virDomainListAllSnapshots, METH_VARARGS, NULL}, {(char *) "virDomainSnapshotListChildrenNames", libvirt_virDomainSnapshotListChildrenNames, METH_VARARGS, NULL}, + {(char *) "virDomainSnapshotListAllChildren", libvirt_virDomainSnapshotListAllChildren, METH_VARARGS, NULL}, {(char *) "virDomainRevertToSnapshot", libvirt_virDomainRevertToSnapshot, METH_VARARGS, NULL}, {(char *) "virDomainGetBlockJobInfo", libvirt_virDomainGetBlockJobInfo, METH_VARARGS, NULL}, {(char *) "virDomainSetBlockIoTune", libvirt_virDomainSetBlockIoTune, METH_VARARGS, NULL}, -- 1.7.10.2

This idea was first suggested by Daniel Veillard here: https://www.redhat.com/archives/libvir-list/2011-October/msg00353.html Now that I am about to add more complexity to snapshot listing, it makes sense to avoid code duplication and special casing for domain listing (all snapshots) vs. snapshot listing (descendants); adding a metaroot reduces the number of code lines by having the domain listing turn into a descendant listing of the metaroot. Note that this has one minor pessimization - if we are going to list ALL snapshots without filtering, then virHashForeach is more efficient than recursing through the child relationships; restoring that minor optimization will occur in the next patch. * src/conf/domain_conf.h (_virDomainSnapshotObj) (_virDomainSnapshotObjList): Repurpose some fields. (virDomainSnapshotDropParent): Drop unused parameter. * src/conf/domain_conf.c (virDomainSnapshotObjListGetNames) (virDomainSnapshotObjListCount): Simplify. (virDomainSnapshotFindByName, virDomainSnapshotSetRelations) (virDomainSnapshotDropParent): Match new field semantics. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML) (qemuDomainSnapshotReparentChildren, qemuDomainSnapshotDelete): Adjust clients. --- Turns out I had 4 instead of 2 more patches to go. Oops, just noticed an #if 0 leftover in this patch. I'll be respinning a v2 anyways, as I hope to rebase this patch to occur earlier in the series. src/conf/domain_conf.c | 119 +++++++++++++----------------------------------- src/conf/domain_conf.h | 12 ++--- src/qemu/qemu_driver.c | 36 +++++---------- 3 files changed, 49 insertions(+), 118 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44343ee..a653af5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14321,33 +14321,14 @@ int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, char **const names, int maxnames, unsigned int flags) { - struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; - int i; - - if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) - return 0; - - if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names, + maxnames, flags); +#if 0 + if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, &data); - } else { - virDomainSnapshotObjPtr root = snapshots->first_root; - while (root) { - virDomainSnapshotObjListCopyNames(root, root->def->name, &data); - root = root->sibling; - } - } - if (data.oom) { - virReportOOMError(); - goto cleanup; - } - - return data.numnames; - -cleanup: - for (i = 0; i < data.numnames; i++) - VIR_FREE(data.names[i]); - return -1; +#endif } int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, @@ -14404,24 +14385,8 @@ static void virDomainSnapshotObjListCount(void *payload, int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, unsigned int flags) { - struct virDomainSnapshotNumData data = { 0, 0 }; - - if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) - return 0; - - if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { - virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data); - } else if (data.flags) { - virDomainSnapshotObjPtr root = snapshots->first_root; - while (root) { - virDomainSnapshotObjListCount(root, root->def->name, &data); - root = root->sibling; - } - } else { - data.count = snapshots->nroots; - } - - return data.count; + flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags); } int @@ -14450,7 +14415,7 @@ virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name) { - return virHashLookup(snapshots->objs, name); + return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot; } void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, @@ -14536,34 +14501,27 @@ virDomainSnapshotSetRelations(void *payload, struct snapshot_set_relation *curr = data; virDomainSnapshotObjPtr tmp; - if (obj->def->parent) { - obj->parent = virDomainSnapshotFindByName(curr->snapshots, - obj->def->parent); - if (!obj->parent) { - curr->err = -1; - VIR_WARN("snapshot %s lacks parent", obj->def->name); - } else { - tmp = obj->parent; - while (tmp) { - if (tmp == obj) { - curr->err = -1; - obj->parent = NULL; - VIR_WARN("snapshot %s in circular chain", obj->def->name); - break; - } - tmp = tmp->parent; - } - if (!tmp) { - obj->parent->nchildren++; - obj->sibling = obj->parent->first_child; - obj->parent->first_child = obj; + obj->parent = virDomainSnapshotFindByName(curr->snapshots, + obj->def->parent); + if (!obj->parent) { + curr->err = -1; + obj->parent = &curr->snapshots->metaroot; + VIR_WARN("snapshot %s lacks parent", obj->def->name); + } else { + tmp = obj->parent; + while (tmp && tmp->def) { + if (tmp == obj) { + curr->err = -1; + obj->parent = &curr->snapshots->metaroot; + VIR_WARN("snapshot %s in circular chain", obj->def->name); + break; } + tmp = tmp->parent; } - } else { - curr->snapshots->nroots++; - obj->sibling = curr->snapshots->first_root; - curr->snapshots->first_root = obj; } + obj->parent->nchildren++; + obj->sibling = obj->parent->first_child; + obj->parent->first_child = obj; } /* Populate parent link and child count of all snapshots, with all @@ -14583,28 +14541,13 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) * of a parent, it is faster to just 0 the count rather than calling * this function on each child. */ void -virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot) +virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) { virDomainSnapshotObjPtr prev = NULL; virDomainSnapshotObjPtr curr = NULL; - size_t *count; - virDomainSnapshotObjPtr *first; - if (snapshot->parent) { - count = &snapshot->parent->nchildren; - first = &snapshot->parent->first_child; - } else { - count = &snapshots->nroots; - first = &snapshots->first_root; - } - - if (!*count || !*first) { - VIR_WARN("inconsistent snapshot relations"); - return; - } - (*count)--; - curr = *first; + snapshot->parent->nchildren--; + curr = snapshot->parent->first_child; while (curr != snapshot) { if (!curr) { VIR_WARN("inconsistent snapshot relations"); @@ -14616,7 +14559,7 @@ virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, if (prev) prev->sibling = snapshot->sibling; else - *first = snapshot->sibling; + snapshot->parent->first_child = snapshot->sibling; snapshot->parent = NULL; snapshot->sibling = NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 88674eb..e3a3679 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1726,9 +1726,11 @@ struct _virDomainSnapshotDef { typedef struct _virDomainSnapshotObj virDomainSnapshotObj; typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; struct _virDomainSnapshotObj { - virDomainSnapshotDefPtr def; + virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */ - virDomainSnapshotObjPtr parent; /* NULL if root */ + virDomainSnapshotObjPtr parent; /* non-NULL except for metaroot, before + virDomainSnapshotUpdateRelations, or + after virDomainSnapshotDropParent */ virDomainSnapshotObjPtr sibling; /* NULL if last child of parent */ size_t nchildren; virDomainSnapshotObjPtr first_child; /* NULL if no children */ @@ -1741,8 +1743,7 @@ struct _virDomainSnapshotObjList { * for O(1), lockless lookup-by-name */ virHashTable *objs; - size_t nroots; - virDomainSnapshotObjPtr first_root; + virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ }; typedef enum { @@ -1788,8 +1789,7 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); -void virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); /* Guest VM runtime state */ typedef struct _virDomainStateReason virDomainStateReason; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1b76c2..7038a4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10480,7 +10480,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* Drop and rebuild the parent relationship, but keep all * child relations by reusing snap. */ - virDomainSnapshotDropParent(&vm->snapshots, other); + virDomainSnapshotDropParent(other); virDomainSnapshotDefFree(other->def); other->def = NULL; snap = other; @@ -10589,18 +10589,12 @@ cleanup: } else { if (update_current) vm->current_snapshot = snap; - if (snap->def->parent) { - other = virDomainSnapshotFindByName(&vm->snapshots, - snap->def->parent); - snap->parent = other; - other->nchildren++; - snap->sibling = other->first_child; - other->first_child = snap; - } else { - vm->snapshots.nroots++; - snap->sibling = vm->snapshots.first_root; - vm->snapshots.first_root = snap; - } + other = virDomainSnapshotFindByName(&vm->snapshots, + snap->def->parent); + snap->parent = other; + other->nchildren++; + snap->sibling = other->first_child; + other->first_child = snap; } } else if (snap) { virDomainSnapshotObjListRemove(&vm->snapshots, snap); @@ -11405,7 +11399,7 @@ qemuDomainSnapshotReparentChildren(void *payload, VIR_FREE(snap->def->parent); snap->parent = rep->parent; - if (rep->parent) { + if (rep->parent->def) { snap->def->parent = strdup(rep->parent->def->name); if (snap->def->parent == NULL) { @@ -11513,15 +11507,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (rep.err < 0) goto endjob; /* Can't modify siblings during ForEachChild, so do it now. */ - if (snap->parent) { - snap->parent->nchildren += snap->nchildren; - rep.last->sibling = snap->parent->first_child; - snap->parent->first_child = snap->first_child; - } else { - vm->snapshots.nroots += snap->nchildren; - rep.last->sibling = vm->snapshots.first_root; - vm->snapshots.first_root = snap->first_child; - } + snap->parent->nchildren += snap->nchildren; + rep.last->sibling = snap->parent->first_child; + snap->parent->first_child = snap->first_child; } if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { @@ -11529,7 +11517,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, snap->first_child = NULL; ret = 0; } else { - virDomainSnapshotDropParent(&vm->snapshots, snap); + virDomainSnapshotDropParent(snap); ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); } -- 1.7.10.2

Wraps the conversion from 'char *name' to virDomainSnapshotPtr in a reusable manner. * src/conf/virdomainlist.h (virDomainListSnapshots): New declaration. * src/conf/virdomainlist.c (virDomainListSnapshots): Implement it. * src/libvirt_private.syms (virdomainlist.h): Export it. --- Assume's Peter's patch has already been applied: https://www.redhat.com/archives/libvir-list/2012-June/msg00543.html src/conf/virdomainlist.c | 41 +++++++++++++++++++++++++++++++++++++++++ src/conf/virdomainlist.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 48 insertions(+) diff --git a/src/conf/virdomainlist.c b/src/conf/virdomainlist.c index 246b838..a6aaec7 100644 --- a/src/conf/virdomainlist.c +++ b/src/conf/virdomainlist.c @@ -180,3 +180,44 @@ cleanup: VIR_FREE(data.domains); return ret; } + +int +virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + virDomainPtr dom, + virDomainSnapshotPtr **snaps, + unsigned int flags) +{ + int count = virDomainSnapshotObjListNum(snapshots, from, flags); + virDomainSnapshotPtr *list; + char **names; + int ret = -1; + int i; + + if (!snaps) + return count; + if (VIR_ALLOC_N(names, count) < 0 || + VIR_ALLOC_N(list, count + 1) < 0) { + virReportOOMError(); + goto cleanup; + } + + virDomainSnapshotObjListGetNames(snapshots, from, names, count, flags); + for (i = 0; i < count; i++) + if ((list[i] = virGetDomainSnapshot(dom, names[i])) == NULL) + goto cleanup; + + ret = count; + *snaps = list; + +cleanup: + for (i = 0; i < count; i++) + VIR_FREE(names[i]); + VIR_FREE(names); + if (ret < 0 && list) { + if (list[i]) + virDomainSnapshotFree(list[i]); + VIR_FREE(list); + } + return ret; +} diff --git a/src/conf/virdomainlist.h b/src/conf/virdomainlist.h index caee592..138d622 100644 --- a/src/conf/virdomainlist.h +++ b/src/conf/virdomainlist.h @@ -63,4 +63,10 @@ int virDomainList(virConnectPtr conn, virHashTablePtr domobjs, virDomainPtr **domains, unsigned int flags); +int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, + virDomainSnapshotObjPtr from, + virDomainPtr dom, + virDomainSnapshotPtr **snaps, + unsigned int flags); + #endif /* __VIR_DOMAIN_LIST_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b37fe68..2fe5068 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1239,6 +1239,7 @@ virDBusGetSystemBus; # virdomainlist.h virDomainList; +virDomainListSnapshots; # virfile.h -- 1.7.10.2