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

[libvirt] [PATCH 2/2] virsh-snapshot: Refactor virsh snapshot-list



Simplify error handling and mutually exlusive option checking.
---

Notes:
    I'm thinking of making the VSH_EXCLUSIVE_OPTION macro global
    in virsh. Mutually exclusive parameters are checked in many
    places throughout virsh and this might really simplify stuff
    sometimes.

 tools/virsh-snapshot.c | 125 ++++++++++++++++++++-----------------------------
 1 file changed, 51 insertions(+), 74 deletions(-)

diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index ed41014..d3ee6c9 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1571,66 +1571,42 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
     virDomainPtr dom = NULL;
     bool ret = false;
     unsigned int flags = 0;
-    bool show_parent = false;
     int i;
     xmlDocPtr xml = NULL;
     xmlXPathContextPtr ctxt = NULL;
     char *doc = NULL;
     virDomainSnapshotPtr snapshot = NULL;
     char *state = NULL;
-    char *parent = NULL;
     long long creation_longlong;
     time_t creation_time_t;
     char timestr[100];
     struct tm time_info;
     bool tree = vshCommandOptBool(cmd, "tree");
     bool name = vshCommandOptBool(cmd, "name");
-    const char *from = NULL;
+    bool from = vshCommandOptBool(cmd, "from");
+    bool parent = vshCommandOptBool(cmd, "parent");
+    bool roots = vshCommandOptBool(cmd, "roots");
+    bool current = vshCommandOptBool(cmd, "current");
+    const char *from_snap = NULL;
+    char *parent_snap = NULL;
     virDomainSnapshotPtr start = NULL;
     vshSnapshotListPtr snaplist = NULL;

-    if (tree && name) {
-        vshError(ctl, "%s",
-                 _("--tree and --name are mutually exclusive"));
-        return false;
+#define VSH_EXCLUSIVE_OPTIONS(NAME1, NAME2)                               \
+    if (NAME1 && NAME2) {                                                 \
+        vshError(ctl, _("Options --%s and --%s are mutually exclusive"),  \
+                 #NAME1, #NAME2);                                         \
+        return false;                                                     \
     }

-    dom = vshCommandOptDomain(ctl, cmd, NULL);
-    if (dom == NULL)
-        goto cleanup;
-
-    if ((vshCommandOptBool(cmd, "from") ||
-         vshCommandOptBool(cmd, "current")) &&
-        vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from) < 0)
-        goto cleanup;
+    VSH_EXCLUSIVE_OPTIONS(tree, name);
+    VSH_EXCLUSIVE_OPTIONS(parent, roots);
+    VSH_EXCLUSIVE_OPTIONS(parent, tree);
+    VSH_EXCLUSIVE_OPTIONS(roots, tree);
+    VSH_EXCLUSIVE_OPTIONS(roots, from);
+    VSH_EXCLUSIVE_OPTIONS(roots, current);
+#undef VSH_EXCLUSIVE_OPTIONS

-    if (vshCommandOptBool(cmd, "parent")) {
-        if (vshCommandOptBool(cmd, "roots")) {
-            vshError(ctl, "%s",
-                     _("--parent and --roots are mutually exclusive"));
-            goto cleanup;
-        }
-        if (tree) {
-            vshError(ctl, "%s",
-                     _("--parent and --tree are mutually exclusive"));
-            goto cleanup;
-        }
-        show_parent = true;
-    } else if (vshCommandOptBool(cmd, "roots")) {
-        if (tree) {
-            vshError(ctl, "%s",
-                     _("--roots and --tree are mutually exclusive"));
-            goto cleanup;
-        }
-        if (from) {
-            vshError(ctl, "%s",
-                     vshCommandOptBool(cmd, "current") ?
-                     _("--roots and --current are mutually exclusive") :
-                     _("--roots and --from are mutually exclusive"));
-            goto cleanup;
-        }
-        flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
-    }
 #define FILTER(option, flag)                                          \
     do {                                                              \
         if (vshCommandOptBool(cmd, option)) {                         \
@@ -1638,7 +1614,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
                 vshError(ctl,                                         \
                          _("--%s and --tree are mutually exclusive"), \
                          option);                                     \
-                goto cleanup;                                         \
+                return false;                                         \
             }                                                         \
             flags |= VIR_DOMAIN_SNAPSHOT_LIST_ ## flag;               \
         }                                                             \
@@ -1653,28 +1629,36 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
     FILTER("external", EXTERNAL);
 #undef FILTER

-    if (vshCommandOptBool(cmd, "metadata")) {
+    if (roots)
+        flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
+
+    if (vshCommandOptBool(cmd, "metadata"))
         flags |= VIR_DOMAIN_SNAPSHOT_LIST_METADATA;
-    }
-    if (vshCommandOptBool(cmd, "no-metadata")) {
+
+    if (vshCommandOptBool(cmd, "no-metadata"))
         flags |= VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA;
-    }

     if (vshCommandOptBool(cmd, "descendants")) {
-        if (!from) {
+        if (!from && !current) {
             vshError(ctl, "%s",
                      _("--descendants requires either --from or --current"));
-            goto cleanup;
+            return false;
         }
         flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
     }

-    if ((snaplist = vshSnapshotListCollect(ctl, dom, start, flags,
-                                           tree)) == NULL)
+    if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
+        return false;
+
+    if ((from || current) &&
+        vshLookupSnapshot(ctl, cmd, "from", true, dom, &start, &from_snap) < 0)
+        goto cleanup;
+
+    if (!(snaplist = vshSnapshotListCollect(ctl, dom, start, flags, tree)))
         goto cleanup;

     if (!tree && !name) {
-        if (show_parent)
+        if (parent)
             vshPrintExtra(ctl, " %-20s %-25s %-15s %s",
                           _("Name"), _("Creation Time"), _("State"),
                           _("Parent"));
@@ -1682,12 +1666,8 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
             vshPrintExtra(ctl, " %-20s %-25s %s",
                           _("Name"), _("Creation Time"), _("State"));
         vshPrintExtra(ctl, "\n"
-"------------------------------------------------------------\n");
-    }
-
-    if (!snaplist->nsnaps) {
-        ret = true;
-        goto cleanup;
+                           "------------------------------"
+                           "------------------------------\n");
     }

     if (tree) {
@@ -1705,7 +1685,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
         const char *snap_name;

         /* free up memory from previous iterations of the loop */
-        VIR_FREE(parent);
+        VIR_FREE(parent_snap);
         VIR_FREE(state);
         xmlXPathFreeContext(ctxt);
         xmlFreeDoc(xml);
@@ -1716,25 +1696,24 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
         assert(snap_name);

         if (name) {
+            /* just print the snapshot name */
             vshPrint(ctl, "%s\n", snap_name);
             continue;
         }

-        doc = virDomainSnapshotGetXMLDesc(snapshot, 0);
-        if (!doc)
+        if (!(doc = virDomainSnapshotGetXMLDesc(snapshot, 0)))
             continue;

-        xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt);
-        if (!xml)
+        if (!(xml = virXMLParseStringCtxt(doc, _("(domain_snapshot)"), &ctxt)))
             continue;

-        if (show_parent)
-            parent = virXPathString("string(/domainsnapshot/parent/name)",
-                                    ctxt);
+        if (parent)
+            parent_snap = virXPathString("string(/domainsnapshot/parent/name)",
+                                         ctxt);

-        state = virXPathString("string(/domainsnapshot/state)", ctxt);
-        if (state == NULL)
+        if (!(state = virXPathString("string(/domainsnapshot/state)", ctxt)))
             continue;
+
         if (virXPathLongLong("string(/domainsnapshot/creationTime)", ctxt,
                              &creation_longlong) < 0)
             continue;
@@ -1749,7 +1728,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)

         if (parent)
             vshPrint(ctl, " %-20s %-25s %-15s %s\n",
-                     snap_name, timestr, state, parent);
+                     snap_name, timestr, state, parent_snap);
         else
             vshPrint(ctl, " %-20s %-25s %s\n", snap_name, timestr, state);
     }
@@ -1759,15 +1738,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd)
 cleanup:
     /* this frees up memory from the last iteration of the loop */
     vshSnapshotListFree(snaplist);
-    VIR_FREE(parent);
+    VIR_FREE(parent_snap);
     VIR_FREE(state);
-    if (start)
-        virDomainSnapshotFree(start);
+    virDomainSnapshotFree(start);
     xmlXPathFreeContext(ctxt);
     xmlFreeDoc(xml);
     VIR_FREE(doc);
-    if (dom)
-        virDomainFree(dom);
+    virDomainFree(dom);

     return ret;
 }
-- 
1.8.1.1


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