[libvirt] [PATCH 0/4] add more snapshot-list filters
Depends on this patch being applied first: https://www.redhat.com/archives/libvir-list/2012-November/msg00598.html Adds the ability for virsh to filter on whether a snapshot was offline, online, or disk-only; and whether it was internal or external. Eric Blake (4): snapshot: add two more filter sets to API snapshot: add virsh back-compat support for new filters snapshot: expose location through virsh snapshot-info snapshot: implement new filter sets include/libvirt/libvirt.h.in | 19 +++++ src/conf/snapshot_conf.c | 30 ++++++- src/conf/snapshot_conf.h | 13 ++- src/libvirt.c | 60 ++++++++++++++ tools/virsh-snapshot.c | 187 +++++++++++++++++++++++++++++++++++++------ tools/virsh.pod | 17 +++- 6 files changed, 298 insertions(+), 28 deletions(-) -- 1.7.11.7
As we enable more modes of snapshot creation, it becomes more important to be able to quickly filter based on snapshot properties. This patch introduces new filter flags; subsequent patches will introduce virsh back-compat filtering, as well as actual libvirt filtering. * include/libvirt/libvirt.h.in (virDomainSnapshotListFlags): Add five new flags in two new groups. * src/libvirt.c (virDomainSnapshotNum, virDomainSnapshotListNames) (virDomainListAllSnapshots, virDomainSnapshotNumChildren) (virDomainSnapshotListChildrenNames) (virDomainSnapshotListAllChildren): Document them. * src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_FILTERS_*): Add new convenience filter collection macros. * tools/virsh-snapshot.c (cmdSnapshotList): Add 5 new flags. * tools/virsh.pod (snapshot-list): Document them. --- include/libvirt/libvirt.h.in | 19 ++++++++++++++ src/conf/snapshot_conf.h | 9 +++++++ src/libvirt.c | 60 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-snapshot.c | 44 +++++++++++++++++++------------- tools/virsh.pod | 17 ++++++++++++- 5 files changed, 130 insertions(+), 19 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 49a361a..75c7c11 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3818,6 +3818,25 @@ typedef enum { which have metadata */ VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA = (1 << 4), /* Filter by snapshots with no metadata */ + + VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE = (1 << 5), /* Filter by snapshots + taken while guest was + offline */ + VIR_DOMAIN_SNAPSHOT_LIST_ONLINE = (1 << 6), /* Filter by snapshots + taken while guest was + online, and with + memory state */ + VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY = (1 << 7), /* Filter by snapshots + taken while guest was + online, but without + memory state */ + + VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL = (1 << 8), /* Filter by snapshots + stored internal to + disk images */ + VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL = (1 << 9), /* Filter by snapshots + that use files external + to disk images */ } virDomainSnapshotListFlags; /* Return the number of snapshots for this domain */ diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index b5c6e25..1aacdc1 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -145,6 +145,15 @@ void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); (VIR_DOMAIN_SNAPSHOT_LIST_LEAVES | \ VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) +# define VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS \ + (VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE | \ + VIR_DOMAIN_SNAPSHOT_LIST_ONLINE | \ + VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) + +# define VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION \ + (VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL | \ + VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL) + # define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \ (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA | \ VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) diff --git a/src/libvirt.c b/src/libvirt.c index bdb1dc6..f1c018e 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17960,6 +17960,16 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE, + * VIR_DOMAIN_SNAPSHOT_LIST_ONLINE, and VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, + * for filtering snapshots based on what domain state is tracked by the + * snapshot. + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL and + * VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, for filtering snapshots based on + * whether the snapshot is stored inside the disk images or as + * additional files. + * * Returns the number of domain snapshots found or -1 in case of error. */ int @@ -18024,6 +18034,16 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE, + * VIR_DOMAIN_SNAPSHOT_LIST_ONLINE, and VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, + * for filtering snapshots based on what domain state is tracked by the + * snapshot. + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL and + * VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, for filtering snapshots based on + * whether the snapshot is stored inside the disk images or as + * additional files. + * * 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 @@ -18106,6 +18126,16 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE, + * VIR_DOMAIN_SNAPSHOT_LIST_ONLINE, and VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, + * for filtering snapshots based on what domain state is tracked by the + * snapshot. + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL and + * VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, for filtering snapshots based on + * whether the snapshot is stored inside the disk images or as + * additional files. + * * 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 @@ -18176,6 +18206,16 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE, + * VIR_DOMAIN_SNAPSHOT_LIST_ONLINE, and VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, + * for filtering snapshots based on what domain state is tracked by the + * snapshot. + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL and + * VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, for filtering snapshots based on + * whether the snapshot is stored inside the disk images or as + * additional files. + * * Returns the number of domain snapshots found or -1 in case of error. */ int @@ -18242,6 +18282,16 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE, + * VIR_DOMAIN_SNAPSHOT_LIST_ONLINE, and VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, + * for filtering snapshots based on what domain state is tracked by the + * snapshot. + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL and + * VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, for filtering snapshots based on + * whether the snapshot is stored inside the disk images or as + * additional files. + * * 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() @@ -18329,6 +18379,16 @@ error: * whether they have metadata that would prevent the removal of the last * reference to a domain. * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE, + * VIR_DOMAIN_SNAPSHOT_LIST_ONLINE, and VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, + * for filtering snapshots based on what domain state is tracked by the + * snapshot. + * + * The next group of @flags is VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL and + * VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, for filtering snapshots based on + * whether the snapshot is stored inside the disk images or as + * additional files. + * * 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 diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 4281109..bc9ffc7 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -1189,6 +1189,11 @@ static const vshCmdOptDef opts_snapshot_list[] = { 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")}, + {"offline", VSH_OT_BOOL, 0, N_("filter by offline snapshots")}, + {"online", VSH_OT_BOOL, 0, N_("filter by online snapshots")}, + {"disk-only", VSH_OT_BOOL, 0, N_("filter by disk-only snapshots")}, + {"internal", VSH_OT_BOOL, 0, N_("filter by internal snapshots")}, + {"external", VSH_OT_BOOL, 0, N_("filter by external snapshots")}, {"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, @@ -1216,8 +1221,6 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) char timestr[100]; 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; vshSnapshotListPtr snaplist = NULL; @@ -1258,22 +1261,27 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) } flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; } - if (leaves) { - if (tree) { - vshError(ctl, "%s", - _("--leaves and --tree are mutually exclusive")); - goto cleanup; - } - 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; - } +#define FILTER(option, flag) \ + do { \ + if (vshCommandOptBool(cmd, option)) { \ + if (tree) { \ + vshError(ctl, \ + _("--%s and --tree are mutually exclusive"), \ + option); \ + goto cleanup; \ + } \ + flags |= VIR_DOMAIN_SNAPSHOT_LIST_ ## flag; \ + } \ + } while (0) + + FILTER("leaves", LEAVES); + FILTER("no-leaves", NO_LEAVES); + FILTER("offline", OFFLINE); + FILTER("online", ONLINE); + FILTER("disk-only", DISK_ONLY); + FILTER("internal", INTERNAL); + FILTER("external", EXTERNAL); +#undef FILTER if (vshCommandOptBool(cmd, "metadata")) { flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA; diff --git a/tools/virsh.pod b/tools/virsh.pod index 0984e6e..b95639b 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2775,6 +2775,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<--no-metadata>] [I<--leaves>] [I<--no-leaves>] +[I<--offline>] [I<--online>] [I<--disk-only>] [I<--internal>] [I<--external>] List all of the available snapshots for the given domain, defaulting to show columns for the snapshot name, creation time, and domain state. @@ -2801,7 +2802,7 @@ 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>. +Filtering 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 @@ -2810,6 +2811,20 @@ 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. +If I<--offline> is specified, the list will be filtered to snapshots +that were taken when the domain was shut off. If I<--online> is +specified, the list will be filtered to snapshots that were taken +when the domain was running, and where the snapshot includes the +memory state to revert to that running state. If I<--disk-only> is +specified, the list will be filtered to snapshots that were taken +when the domain was running, but where the snapshot includes only +disk state. + +If I<--internal> is specified, the list will be filtered to snapshots +that use internal storage of existing disk images. If I<--external> +is specified, the list will be filtered to snapshots that use external +files for disk images or memory state. + =item B<snapshot-dumpxml> I<domain> I<snapshot> [I<--security-info>] Output the snapshot XML for the domain's snapshot named I<snapshot>. -- 1.7.11.7
On 11/13/12 20:09, Eric Blake wrote:
As we enable more modes of snapshot creation, it becomes more important to be able to quickly filter based on snapshot properties. This patch introduces new filter flags; subsequent patches will introduce virsh back-compat filtering, as well as actual libvirt filtering.
* include/libvirt/libvirt.h.in (virDomainSnapshotListFlags): Add five new flags in two new groups. * src/libvirt.c (virDomainSnapshotNum, virDomainSnapshotListNames) (virDomainListAllSnapshots, virDomainSnapshotNumChildren) (virDomainSnapshotListChildrenNames) (virDomainSnapshotListAllChildren): Document them. * src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_FILTERS_*): Add new convenience filter collection macros. * tools/virsh-snapshot.c (cmdSnapshotList): Add 5 new flags. * tools/virsh.pod (snapshot-list): Document them. --- include/libvirt/libvirt.h.in | 19 ++++++++++++++ src/conf/snapshot_conf.h | 9 +++++++ src/libvirt.c | 60 ++++++++++++++++++++++++++++++++++++++++++++ tools/virsh-snapshot.c | 44 +++++++++++++++++++------------- tools/virsh.pod | 17 ++++++++++++- 5 files changed, 130 insertions(+), 19 deletions(-)
diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 49a361a..75c7c11 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -3818,6 +3818,25 @@ typedef enum { which have metadata */ VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA = (1 << 4), /* Filter by snapshots with no metadata */ + + VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE = (1 << 5), /* Filter by snapshots + taken while guest was + offline */ + VIR_DOMAIN_SNAPSHOT_LIST_ONLINE = (1 << 6), /* Filter by snapshots + taken while guest was + online, and with + memory state */
I'm wondering if we shouldn't go for ACTIVE/INACTIVE here?
+ VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY = (1 << 7), /* Filter by snapshots + taken while guest was + online, but without + memory state */ + + VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL = (1 << 8), /* Filter by snapshots + stored internal to + disk images */ + VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL = (1 << 9), /* Filter by snapshots + that use files external + to disk images */ } virDomainSnapshotListFlags;
/* Return the number of snapshots for this domain */
Otherwise looks okay so I'm comfortable with ACKing this when the naming question gets settled. Peter
On 11/13/2012 04:50 PM, Peter Krempa wrote:
On 11/13/12 20:09, Eric Blake wrote:
As we enable more modes of snapshot creation, it becomes more important to be able to quickly filter based on snapshot properties. This patch introduces new filter flags; subsequent patches will introduce virsh back-compat filtering, as well as actual libvirt filtering.
+++ b/include/libvirt/libvirt.h.in @@ -3818,6 +3818,25 @@ typedef enum { which have metadata */ VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA = (1 << 4), /* Filter by snapshots with no metadata */ + + VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE = (1 << 5), /* Filter by snapshots + taken while guest was + offline */ + VIR_DOMAIN_SNAPSHOT_LIST_ONLINE = (1 << 6), /* Filter by snapshots + taken while guest was + online, and with + memory state */
I'm wondering if we shouldn't go for ACTIVE/INACTIVE here?
Indeed, that is more consistent with what we have used elsewhere. I'll respin the series with that change.
Otherwise looks okay so I'm comfortable with ACKing this when the naming question gets settled.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Snapshot filtering based on types is useful enough to add back-compat support into virsh. It is also rather easy - all versions of libvirt that don't understand the new filter flags already gave us sufficient information in a single XML field to reconstruct all the information we need (that is, it isn't until libvirt 1.0.1 that we have more interesting types of snapshots, such as offline external). * tools/virsh-snapshot.c (vshSnapshotFilter): New function. (vshSnapshotListCollect): Add fallback support. --- tools/virsh-snapshot.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index bc9ffc7..d5c71be 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -39,6 +39,7 @@ #include "util.h" #include "virsh-domain.h" #include "xml.h" +#include "conf/snapshot_conf.h" /* Helper for snapshot-create and snapshot-create-as */ static bool @@ -712,6 +713,67 @@ cleanup: return ret; } +/* Helper function to filter snapshots according to status and + * location portion of flags. Returns 0 if filter excluded snapshot + * (or if snapshot is already NULL), 1 if snapshot is okay, and -1 on + * failure with error reported. */ +static int +vshSnapshotFilter(vshControl *ctl, virDomainSnapshotPtr snapshot, + unsigned int flags) +{ + char *xml = NULL; + xmlDocPtr xmldoc = NULL; + xmlXPathContextPtr ctxt = NULL; + int ret = -1; + char *state = NULL; + + if (!snapshot) + return 0; + + xml = virDomainSnapshotGetXMLDesc(snapshot, 0); + if (!xml) + goto cleanup; + + xmldoc = virXMLParseStringCtxt(xml, _("(domain_snapshot)"), &ctxt); + if (!xmldoc) + goto cleanup; + + /* Libvirt 1.0.1 and newer never call this function, because the + * filtering is already supported by the listing functions. Older + * libvirt lacked /domainsnapshot/memory, but was also limited in + * the types of snapshots it could create: if state was disk-only, + * the snapshot is external; all other snapshots are internal. */ + state = virXPathString("string(/domainsnapshot/state)", ctxt); + if (!state) + goto cleanup; + if (STREQ(state, "disk-snapshot")) { + ret = ((flags & (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY | + VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)) == + (VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY | + VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL)); + } else { + if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL)) + ret = 0; + else if (STREQ(state, "shutoff")) + ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE); + else + ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ONLINE); + } + +cleanup: + if (ret < 0) { + vshReportError(ctl); + vshError(ctl, "%s", _("unable to perform snapshot filtering")); + } else { + vshResetLibvirtError(); + } + VIR_FREE(state); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmldoc); + VIR_FREE(xml); + return ret; +} + /* * "snapshot-info" command */ @@ -883,7 +945,7 @@ vshSnapSorter(const void *a, const void *b) static vshSnapshotListPtr vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, virDomainSnapshotPtr from, - unsigned int flags, bool tree) + unsigned int orig_flags, bool tree) { int i; char **names = NULL; @@ -896,6 +958,8 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, const char *fromname = NULL; int start_index = -1; int deleted = 0; + bool filter_fallback = false; + unsigned int flags = orig_flags; /* Try the interface available in 0.9.13 and newer. */ if (!ctl->useSnapshotOld) { @@ -903,6 +967,20 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, count = virDomainSnapshotListAllChildren(from, &snaps, flags); else count = virDomainListAllSnapshots(dom, &snaps, flags); + /* If we failed because of flags added in 1.0.1, we can do + * fallback filtering. */ + if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG && + flags & (VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS | + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION)) { + flags &= ~(VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS | + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION); + vshResetLibvirtError(); + filter_fallback = true; + if (from) + count = virDomainSnapshotListAllChildren(from, &snaps, flags); + else + count = virDomainListAllSnapshots(dom, &snaps, flags); + } } if (count >= 0) { /* When mixing --from and --tree, we also want a copy of from @@ -950,6 +1028,12 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, return snaplist; flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA; } + if (flags & (VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS | + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION)) { + flags &= ~(VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS | + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION); + filter_fallback = true; + } /* This uses the interfaces available in 0.8.0-0.9.6 * (virDomainSnapshotListNames, global list only) and in @@ -1144,6 +1228,31 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom, } success: + if (filter_fallback) { + /* Older API didn't filter on status or location, but the + * information is available in domain XML. */ + if (!(orig_flags & VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS)) + orig_flags |= VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS; + if (!(orig_flags & VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION)) + orig_flags |= VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION; + for (i = 0; i < snaplist->nsnaps; i++) { + switch (vshSnapshotFilter(ctl, snaplist->snaps[i].snap, + orig_flags)) { + case 1: + break; + case 0: + if (snaplist->snaps[i].snap) { + virDomainSnapshotFree(snaplist->snaps[i].snap); + snaplist->snaps[i].snap = NULL; + VIR_FREE(snaplist->snaps[i].parent); + deleted++; + } + break; + default: + goto cleanup; + } + } + } qsort(snaplist->snaps, snaplist->nsnaps, sizeof(*snaplist->snaps), vshSnapSorter); snaplist->nsnaps -= deleted; -- 1.7.11.7
On 11/13/12 20:09, Eric Blake wrote:
Snapshot filtering based on types is useful enough to add back-compat support into virsh. It is also rather easy - all versions of libvirt that don't understand the new filter flags already gave us sufficient information in a single XML field to reconstruct all the information we need (that is, it isn't until libvirt 1.0.1 that we have more interesting types of snapshots, such as offline external).
* tools/virsh-snapshot.c (vshSnapshotFilter): New function. (vshSnapshotListCollect): Add fallback support. --- tools/virsh-snapshot.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-)
ACK. Peter
Now that we can filter on this information, we should also make it easy to get at. * tools/virsh-snapshot.c (cmdSnapshotInfo): Add another output row. --- tools/virsh-snapshot.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index d5c71be..da1e75f 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -797,7 +797,8 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotPtr snapshot = NULL; const char *name; char *doc = NULL; - char *tmp; + char *state; + bool internal; char *parent = NULL; bool ret = false; int count; @@ -839,15 +840,36 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) if (!doc) goto cleanup; - tmp = strstr(doc, "<state>"); - if (!tmp) { + state = strstr(doc, "<state>"); + if (!state) { vshError(ctl, "%s", _("unexpected problem reading snapshot xml")); goto cleanup; } - tmp += strlen("<state>"); + state += strlen("<state>"); vshPrint(ctl, "%-15s %.*s\n", _("State:"), - (int) (strchr(tmp, '<') - tmp), tmp); + (int) (strchr(state, '<') - state), state); + + /* In addition to state, location is useful. If the snapshot has + * a <memory> element, then the existence of snapshot='external' + * prior to <domain> is the deciding factor; for snapshots + * created prior to 1.0.1, a state of disk-only is the only + * external snapshot. */ + if (strstr(state, "<memory")) { + char *domain = strstr(state, "<domain"); + + if (!domain) { + vshError(ctl, "%s", + _("unexpected problem reading snapshot xml")); + goto cleanup; + } + internal = !memmem(state, domain - state, "snapshot='external'", + strlen("snapshot='external'")); + } else { + internal = !STRPREFIX(state, "disk-snapshot"); + } + vshPrint(ctl, "%-15s %s\n", _("Location:"), + internal ? _("internal") : _("external")); if (vshGetSnapshotParent(ctl, snapshot, &parent) < 0) goto cleanup; -- 1.7.11.7
On 11/13/12 20:09, Eric Blake wrote:
Now that we can filter on this information, we should also make it easy to get at.
* tools/virsh-snapshot.c (cmdSnapshotInfo): Add another output row. --- tools/virsh-snapshot.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index d5c71be..da1e75f 100644 --- a/tools/virsh-snapshot.c +++ b/tools/virsh-snapshot.c @@ -797,7 +797,8 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotPtr snapshot = NULL; const char *name; char *doc = NULL; - char *tmp; + char *state; + bool internal; char *parent = NULL; bool ret = false; int count; @@ -839,15 +840,36 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) if (!doc) goto cleanup;
- tmp = strstr(doc, "<state>"); - if (!tmp) { + state = strstr(doc, "<state>"); + if (!state) { vshError(ctl, "%s", _("unexpected problem reading snapshot xml")); goto cleanup; } - tmp += strlen("<state>"); + state += strlen("<state>"); vshPrint(ctl, "%-15s %.*s\n", _("State:"), - (int) (strchr(tmp, '<') - tmp), tmp); + (int) (strchr(state, '<') - state), state); + + /* In addition to state, location is useful. If the snapshot has + * a <memory> element, then the existence of snapshot='external' + * prior to <domain> is the deciding factor; for snapshots + * created prior to 1.0.1, a state of disk-only is the only + * external snapshot. */ + if (strstr(state, "<memory")) { + char *domain = strstr(state, "<domain");
Bleah. Raw XML parsing. Wouldn't it be easier in and cleaner convert this piece code to use the XML parser and xpath?
+ + if (!domain) { + vshError(ctl, "%s", + _("unexpected problem reading snapshot xml")); + goto cleanup; + } + internal = !memmem(state, domain - state, "snapshot='external'", + strlen("snapshot='external'")); + } else { + internal = !STRPREFIX(state, "disk-snapshot"); + } + vshPrint(ctl, "%-15s %s\n", _("Location:"), + internal ? _("internal") : _("external"));
if (vshGetSnapshotParent(ctl, snapshot, &parent) < 0) goto cleanup;
The code looks OK in what it should be doing, but I don't like the raw XML parse-hacking stuff. The only reason to put this in as-is would be if it would be really complicated/overheading to use xpath here. Peter
On 11/13/2012 05:16 PM, Peter Krempa wrote:
On 11/13/12 20:09, Eric Blake wrote:
Now that we can filter on this information, we should also make it easy to get at.
* tools/virsh-snapshot.c (cmdSnapshotInfo): Add another output row. --- tools/virsh-snapshot.c | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-)
- tmp = strstr(doc, "<state>"); - if (!tmp) { + state = strstr(doc, "<state>"); + if (!state) { vshError(ctl, "%s", _("unexpected problem reading snapshot xml")); goto cleanup; } - tmp += strlen("<state>"); + state += strlen("<state>"); vshPrint(ctl, "%-15s %.*s\n", _("State:"), - (int) (strchr(tmp, '<') - tmp), tmp); + (int) (strchr(state, '<') - state), state); + + /* In addition to state, location is useful. If the snapshot has + * a <memory> element, then the existence of snapshot='external' + * prior to <domain> is the deciding factor; for snapshots + * created prior to 1.0.1, a state of disk-only is the only + * external snapshot. */ + if (strstr(state, "<memory")) { + char *domain = strstr(state, "<domain");
Bleah. Raw XML parsing. Wouldn't it be easier in and cleaner convert this piece code to use the XML parser and xpath?
Not the first time we've done this. I agree that using the XML parser and xpath is probably nicer, but it actually takes more code than a simple strstr.
The code looks OK in what it should be doing, but I don't like the raw XML parse-hacking stuff. The only reason to put this in as-is would be if it would be really complicated/overheading to use xpath here.
I'll post an interdiff that shows what it would take to use xpath, and we can decide based on how nice or ugly it looks. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/14/2012 11:43 AM, Eric Blake wrote:
Bleah. Raw XML parsing. Wouldn't it be easier in and cleaner convert this piece code to use the XML parser and xpath?
Not the first time we've done this. I agree that using the XML parser and xpath is probably nicer, but it actually takes more code than a simple strstr.
The code looks OK in what it should be doing, but I don't like the raw XML parse-hacking stuff. The only reason to put this in as-is would be if it would be really complicated/overheading to use xpath here.
I'll post an interdiff that shows what it would take to use xpath, and we can decide based on how nice or ugly it looks.
Here's the diff; any decisions on whether to go with xpath? tools/virsh-snapshot.c | 61 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git i/tools/virsh-snapshot.c w/tools/virsh-snapshot.c index e38ce84..36f5b46 100644 --- i/tools/virsh-snapshot.c +++ w/tools/virsh-snapshot.c @@ -797,8 +797,10 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) virDomainSnapshotPtr snapshot = NULL; const char *name; char *doc = NULL; - char *state; - bool internal; + xmlDocPtr xmldoc = NULL; + xmlXPathContextPtr ctxt = NULL; + char *state = NULL; + int external; char *parent = NULL; bool ret = false; int count; @@ -840,39 +842,48 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) if (!doc) goto cleanup; - state = strstr(doc, "<state>"); + xmldoc = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt); + if (!xmldoc) + goto cleanup; + + state = virXPathString("string(/domainsnapshot/state)", ctxt); if (!state) { vshError(ctl, "%s", _("unexpected problem reading snapshot xml")); goto cleanup; } - state += strlen("<state>"); - vshPrint(ctl, "%-15s %.*s\n", _("State:"), - (int) (strchr(state, '<') - state), state); + vshPrint(ctl, "%-15s %s\n", _("State:"), state); /* In addition to state, location is useful. If the snapshot has * a <memory> element, then the existence of snapshot='external' * prior to <domain> is the deciding factor; for snapshots * created prior to 1.0.1, a state of disk-only is the only * external snapshot. */ - if (strstr(state, "<memory")) { - char *domain = strstr(state, "<domain"); + switch (virXPathBoolean("boolean(/domainsnapshot/memory)", ctxt)) { + case 1: + external = virXPathBoolean("boolean(/domainsnapshot/memory/@snapshot=external " + "| /domainsnapshot/disks/disk/@snapshot=external)", + ctxt); + break; + case 0: + external = STREQ(state, "disk-snapshot"); + break; + default: + external = -1; + break; - if (!domain) { - vshError(ctl, "%s", - _("unexpected problem reading snapshot xml")); - goto cleanup; - } - internal = !memmem(state, domain - state, "snapshot='external'", - strlen("snapshot='external'")); - } else { - internal = !STRPREFIX(state, "disk-snapshot"); + } + if (external < 0) { + vshError(ctl, "%s", + _("unexpected problem reading snapshot xml")); + goto cleanup; } vshPrint(ctl, "%-15s %s\n", _("Location:"), - internal ? _("internal") : _("external")); + external ? _("external") : _("internal")); - if (vshGetSnapshotParent(ctl, snapshot, &parent) < 0) - goto cleanup; + /* Since we already have the XML, there's no need to call + * virDomainSnapshotGetParent */ + parent = virXPathString("string(/domainsnapshot/parent/name)", ctxt); vshPrint(ctl, "%-15s %s\n", _("Parent:"), parent ? parent : "-"); /* Children, Descendants. After this point, the fallback to @@ -884,8 +895,13 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) } flags = 0; count = virDomainSnapshotNumChildren(snapshot, flags); - if (count < 0) + if (count < 0) { + if (last_error->code == VIR_ERR_NO_SUPPORT) { + vshResetLibvirtError(); + ret = true; + } goto cleanup; + } vshPrint(ctl, "%-15s %d\n", _("Children:"), count); flags = VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS; count = virDomainSnapshotNumChildren(snapshot, flags); @@ -908,6 +924,9 @@ cmdSnapshotInfo(vshControl *ctl, const vshCmd *cmd) ret = true; cleanup: + VIR_FREE(state); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmldoc); VIR_FREE(doc); VIR_FREE(parent); if (snapshot) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/15/12 00:20, Eric Blake wrote:
On 11/14/2012 11:43 AM, Eric Blake wrote:
Bleah. Raw XML parsing. Wouldn't it be easier in and cleaner convert this piece code to use the XML parser and xpath?
Not the first time we've done this. I agree that using the XML parser and xpath is probably nicer, but it actually takes more code than a simple strstr.
The code looks OK in what it should be doing, but I don't like the raw XML parse-hacking stuff. The only reason to put this in as-is would be if it would be really complicated/overheading to use xpath here.
I'll post an interdiff that shows what it would take to use xpath, and we can decide based on how nice or ugly it looks.
Here's the diff; any decisions on whether to go with xpath?
tools/virsh-snapshot.c | 61 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 40 insertions(+), 21 deletions(-)
I definitely like the xpath version better. Peter
Relatively straight-forward. And since qemu was already using VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, with 6 different APIs all calling into this common code, I've instantly added all 5 flags to 6 APIs. * src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_FILTERS_ALL): Enable new filters. * src/conf/snapshot_conf.c (virDomainSnapshotObjListGetNames): Prep the new flags. (virDomainSnapshotObjListCopyNames): Actually do the filtering. --- src/conf/snapshot_conf.c | 30 ++++++++++++++++++++++++++++-- src/conf/snapshot_conf.h | 4 +++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index aa2b526..275445e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -739,6 +739,26 @@ static void virDomainSnapshotObjListCopyNames(void *payload, if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES) && !obj->nchildren) return; + if (data->flags & VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS) { + if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_OFFLINE) && + obj->def->state == VIR_DOMAIN_SHUTOFF) + return; + if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) && + obj->def->state == VIR_DOMAIN_DISK_SNAPSHOT) + return; + if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_ONLINE) && + obj->def->state != VIR_DOMAIN_SHUTOFF && + obj->def->state != VIR_DOMAIN_DISK_SNAPSHOT) + return; + } + + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL) && + virDomainSnapshotIsExternal(obj)) + return; + if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL) && + !virDomainSnapshotIsExternal(obj)) + return; + if (data->names && data->count < data->maxnames && !(data->names[data->count] = strdup(obj->def->name))) { data->error = true; @@ -780,11 +800,17 @@ virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, return 0; data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA; - /* For ease of coding the visitor, it is easier to zero the LEAVES - * group if both bits are set. */ + /* For ease of coding the visitor, it is easier to zero each group + * where all of the bits are set. */ if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) == VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES; + if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS) == + VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS) + data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS; + if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION) == + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION) + data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION; if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) { if (from->def) diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h index 1aacdc1..a408659 100644 --- a/src/conf/snapshot_conf.h +++ b/src/conf/snapshot_conf.h @@ -156,7 +156,9 @@ void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); # define VIR_DOMAIN_SNAPSHOT_FILTERS_ALL \ (VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA | \ - VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES) + VIR_DOMAIN_SNAPSHOT_FILTERS_LEAVES | \ + VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS | \ + VIR_DOMAIN_SNAPSHOT_FILTERS_LOCATION) int virDomainListSnapshots(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr from, -- 1.7.11.7
On 11/13/12 20:09, Eric Blake wrote:
Relatively straight-forward. And since qemu was already using VIR_DOMAIN_SNAPSHOT_FILTERS_ALL, with 6 different APIs all calling into this common code, I've instantly added all 5 flags to 6 APIs.
* src/conf/snapshot_conf.h (VIR_DOMAIN_SNAPSHOT_FILTERS_ALL): Enable new filters. * src/conf/snapshot_conf.c (virDomainSnapshotObjListGetNames): Prep the new flags. (virDomainSnapshotObjListCopyNames): Actually do the filtering. --- src/conf/snapshot_conf.c | 30 ++++++++++++++++++++++++++++-- src/conf/snapshot_conf.h | 4 +++- 2 files changed, 31 insertions(+), 3 deletions(-)
ACK. Peter
participants (2)
-
Eric Blake -
Peter Krempa