[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 2/4] snapshot: add virsh back-compat support for new filters



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]