On 11/15/12 00:36, 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(-) diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c index 0363e19..7cd2966 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_INACTIVE); + else + ret = !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE); + } + +cleanup: + if (ret < 0) { + vshReportError(ctl); + vshError(ctl, "%s", _("unable to perform snapshot filtering"));
Hm, reporting of libvirt's error isn't really needed here. When the filtering fails everything should fail gracefully and the error would be printed by vshCmdRun at the end when the command fails. The only benefit of this is that the error from libvirt is printed before the more specific error. In other commands we just print a local error and then let the libvirt error to be printed by the command handler.
+ } else { + vshResetLibvirtError(); + } + VIR_FREE(state); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmldoc); + VIR_FREE(xml); + return ret; +} +
ACK when you remove the error message printing or provide a explanation why you chose that approach.
Peter -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list