[libvirt] [PATCH 5/6] snapshot: new virsh function factored from snapshot-list

[Peter Krempa]

On 06/12/12 14:54, Peter Krempa wrote:
> On 06/09/12 06:34, Eric Blake wrote:
>> This patch copies just the fallback code out of cmdSnapshotList,
>> and keeps the snapshot objects around rather than just their
>> name for easier manipulation.  It looks forward to a future
>> patch when we add a way to list all snapshot objects at once,
>> and the next patch will simplify cmdSnapshotList to take
>> advantage of this factorization.
>>

> I know it's copied code, but this error message doesn't look helpful. I think it's worth improving: "Failed to collect snapshot list" perhaps?
>
>> +        goto cleanup;
>> +    }
>
> I had a very hard time parsing the code flow in this block, I'd rewrite this block as follows:

I forgot to attach the patch for the rewrite.

Peter
-------------- next part --------------
diff --git a/tools/virsh.c b/tools/virsh.c
index 488f8a4..c3aa92f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -16627,7 +16627,7 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom,
 {
     int i;
     char **names = NULL;
-    int count = 0;
+    int count = -1;
     bool descendants = false;
     bool roots = false;
     vshSnapshotListPtr snaplist = vshMalloc(ctl, sizeof(*snaplist));
@@ -16641,36 +16641,40 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom,
     /* This is the interface available in 0.9.12 and earlier,
      * including fallbacks to 0.9.4 and earlier.  */
     if (from) {
-        fromname = virDomainSnapshotGetName(from);
-        if (!fromname) {
+        if (!(fromname = virDomainSnapshotGetName(from))) {
             vshError(ctl, "%s", _("Could not get snapshot name"));
             goto cleanup;
         }
+
         descendants = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0;
+
         if (tree)
             flags |= VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
-        count = ctl->useSnapshotOld ? -1 :
-            virDomainSnapshotNumChildren(from, flags);
-        if (count < 0) {
-            if (ctl->useSnapshotOld ||
-                last_error->code == VIR_ERR_NO_SUPPORT) {
-                /* We can emulate --from.  */
-                /* XXX can we also emulate --leaves? */
-                virFreeError(last_error);
-                last_error = NULL;
-                ctl->useSnapshotOld = true;
-                flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
-                count = virDomainSnapshotNum(dom, flags);
-            }
-        } else if (tree) {
-            count++;
+
+        /* try new API at first */
+        if (!ctl->useSnapshotOld &&
+           (count = virDomainSnapshotNumChildren(from, flags)) < 0 &&
+            last_error && last_error->code == VIR_ERR_NO_SUPPORT) {
+            /* We can emulate --from.  */
+            /* XXX can we also emulate --leaves? */
+            virFreeError(last_error);
+            lastError = NULL;
+            ctl->useSnapshotOld = true;
+            flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS;
         }
-    } else {
+
+        if (tree && count >= 0)
+            count++;
+    }
+
+    /* fallback to old API */
+    if (count < 0)  {
         count = virDomainSnapshotNum(dom, flags);

         /* Fall back to simulation if --roots was unsupported. */
         /* XXX can we also emulate --leaves? */
-        if (count < 0 && last_error->code == VIR_ERR_INVALID_ARG &&
+        if (!from && count < 0 &&
+            last_error->code == VIR_ERR_INVALID_ARG &&
             (flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
             virFreeError(last_error);
             last_error = NULL;
@@ -16682,7 +16686,7 @@ vshSnapshotListCollect(vshControl *ctl, virDomainPtr dom,

     if (count < 0) {
         if (!last_error)
-            vshError(ctl, _("missing support"));
+            vshError(ctl, _("Failed to collect snapshot list"));
         goto cleanup;
     }