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

Re: [libvirt] [PATCHv2 13/7] snapshot: implement snapshot children listing in vbox



2011/10/7 Eric Blake <eblake redhat com>:
> Adding this for VBox was a bit harder than for ESX, but the same
> principles apply for starting the traversal at a known point
> rather than covering the entire hierarchy.
>
> * src/vbox/vbox_tmpl.c (vboxCountDescendants)
> (vboxDomainSnapshotNumChildren)
> (vboxDomainSnapshotListChildrenNames): New functions.
> ---
>
> Changes in v2: avoid leaking snapshot, and fix recursive children
> names to get through loop properly by transferring initial snapshot
> into loop then skipping that element while grabbing names.
>
> Still untested from my end, but hopefully does better.
>
>  src/vbox/vbox_tmpl.c |  213 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 213 insertions(+), 0 deletions(-)
>
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index c74d2cf..84a4fca 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c

> +static int
> +vboxDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
> +                                    char **names,
> +                                    int nameslen,
> +                                    unsigned int flags)
> +{
> +    virDomainPtr dom = snapshot->domain;
> +    VBOX_OBJECT_CHECK(dom->conn, int, -1);
> +    vboxIID iid = VBOX_IID_INITIALIZER;
> +    IMachine *machine = NULL;
> +    ISnapshot *snap = NULL;
> +    nsresult rc;
> +    ISnapshot **snapshots = NULL;
> +    PRUint32 count = 0;
> +    int i;
> +    vboxArray children = VBOX_ARRAY_INITIALIZER;
> +
> +    virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS |
> +                  VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1);
> +
> +    vboxIIDFromUUID(&iid, dom->uuid);
> +    rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine);
> +    if (NS_FAILED(rc)) {
> +        vboxError(VIR_ERR_NO_DOMAIN, "%s",
> +                  _("no domain with matching UUID"));
> +        goto cleanup;
> +    }
> +
> +    if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name)))
> +        goto cleanup;
> +
> +    if (!nameslen || (flags & VIR_DOMAIN_SNAPSHOT_LIST_METADATA)) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    /* Over-allocates, but this is the easiest way to do things */
> +    rc = machine->vtbl->GetSnapshotCount(machine, &count);
> +    if (NS_FAILED(rc)) {
> +        vboxError(VIR_ERR_INTERNAL_ERROR,
> +                  _("could not get snapshot count for domain %s"),
> +                  dom->name);
> +        goto cleanup;
> +    }
> +
> +    if (VIR_ALLOC_N(snapshots, count) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    rc = vboxArrayGet(&children, snap, snap->vtbl->GetChildren);
> +    if (NS_FAILED(rc)) {
> +        vboxError(VIR_ERR_INTERNAL_ERROR,
> +                  "%s", _("could not get children snapshots"));
> +        goto cleanup;
> +    }
> +
> +    if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) {
> +        int top = children.count;
> +        int next;
> +
> +        snapshots[0] = snap;
> +        snap = NULL;
> +        for (next = 0; next < count; next++) {
> +            if (!snapshots[next])
> +                break;
> +            rc = vboxArrayGet(&children, snapshots[next],
> +                              snapshots[next]->vtbl->GetChildren);
> +            if (NS_FAILED(rc)) {
> +                vboxError(VIR_ERR_INTERNAL_ERROR,
> +                          "%s", _("could not get children snapshots"));
> +                goto cleanup;
> +            }
> +            for (i = 0; i < children.count; i++) {
> +                ISnapshot *child = children.items[i];
> +                if (!child)
> +                    continue;
> +                if (top == count) {
> +                    vboxError(VIR_ERR_INTERNAL_ERROR,
> +                              _("unexpected number of snapshots > %u"), count);
> +                    vboxArrayRelease(&children);
> +                    goto cleanup;
> +                }
> +                VBOX_ADDREF(child);
> +                snapshots[top++] = child;
> +            }
> +            vboxArrayRelease(&children);
> +        }
> +        count = top - 1;
> +    } else {
> +        count = children.count;
> +    }
> +
> +    for (i = 0; i < nameslen; i++) {
> +        PRUnichar *nameUtf16;
> +        char *name;
> +        int j = i + !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS);
> +
> +        if (i >= count)
> +            break;
> +
> +        rc = snapshots[j]->vtbl->GetName(snapshots[j], &nameUtf16);

Still a segfault here because snapshots[0] is NULL. Also I think this
logic is too complicated here. Are we really afraid of a stack
overflow because of a domain with many snapshots that we need to avoid
a recursive solution?

You already implemented vboxCountDescendants recursively, I'd suggest
to do the same here too.

I attached a patch to be applied on top of this one that does this and
works for me.

ACK with the attached patch applied.

-- 
Matthias Bolte
http://photron.blogspot.com
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index fd585c4..4169b02 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -6108,6 +6108,79 @@ cleanup:
 }
 
 static int
+vboxGetDescendantNames(vboxGlobalData *data, ISnapshot *snapshot, char **names,
+                       int nameslen, bool recurse)
+{
+    bool success = false;
+    vboxArray children = VBOX_ARRAY_INITIALIZER;
+    nsresult rc;
+    int count = 0;
+    int result;
+    int i;
+
+    rc = vboxArrayGet(&children, snapshot, snapshot->vtbl->GetChildren);
+    if (NS_FAILED(rc)) {
+        vboxError(VIR_ERR_INTERNAL_ERROR,
+                  "%s", _("could not get children snapshots"));
+        goto cleanup;
+    }
+
+    for (i = 0; i < children.count && count < nameslen; i++) {
+        PRUnichar *nameUtf16;
+        char *name;
+        ISnapshot *child = children.items[i];
+
+        rc = child->vtbl->GetName(child, &nameUtf16);
+        if (NS_FAILED(rc) || !nameUtf16) {
+            vboxError(VIR_ERR_INTERNAL_ERROR,
+                      "%s", _("could not get snapshot name"));
+            goto cleanup;
+        }
+
+        VBOX_UTF16_TO_UTF8(nameUtf16, &name);
+        VBOX_UTF16_FREE(nameUtf16);
+        names[count] = strdup(name);
+        VBOX_UTF8_FREE(name);
+
+        if (names[count] == NULL) {
+            virReportOOMError();
+            goto cleanup;
+        }
+
+        count++;
+
+        if (count >= nameslen) {
+            break;
+        }
+
+        if (recurse) {
+            result = vboxGetDescendantNames(data, child, names + count,
+                                            nameslen - count, true);
+
+            if (result < 0) {
+                goto cleanup;
+            }
+
+            count += result;
+        }
+    }
+
+    success = true;
+
+cleanup:
+    if (!success) {
+        for (i = 0; i < count; ++i) {
+            VIR_FREE(names[i]);
+        }
+
+        count = -1;
+    }
+
+    vboxArrayRelease(&children);
+    return count;
+}
+
+static int
 vboxDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
                                     char **names,
                                     int nameslen,
@@ -6119,14 +6192,13 @@ vboxDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
     IMachine *machine = NULL;
     ISnapshot *snap = NULL;
     nsresult rc;
-    ISnapshot **snapshots = NULL;
-    PRUint32 count = 0;
-    int i;
-    vboxArray children = VBOX_ARRAY_INITIALIZER;
+    bool recurse;
 
     virCheckFlags(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS |
                   VIR_DOMAIN_SNAPSHOT_LIST_METADATA, -1);
 
+    recurse = (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) != 0;
+
     vboxIIDFromUUID(&iid, dom->uuid);
     rc = VBOX_OBJECT_GET_MACHINE(iid.value, &machine);
     if (NS_FAILED(rc)) {
@@ -6143,98 +6215,9 @@ vboxDomainSnapshotListChildrenNames(virDomainSnapshotPtr snapshot,
         goto cleanup;
     }
 
-    /* Over-allocates, but this is the easiest way to do things */
-    rc = machine->vtbl->GetSnapshotCount(machine, &count);
-    if (NS_FAILED(rc)) {
-        vboxError(VIR_ERR_INTERNAL_ERROR,
-                  _("could not get snapshot count for domain %s"),
-                  dom->name);
-        goto cleanup;
-    }
-
-    if (VIR_ALLOC_N(snapshots, count) < 0) {
-        virReportOOMError();
-        goto cleanup;
-    }
-
-    rc = vboxArrayGet(&children, snap, snap->vtbl->GetChildren);
-    if (NS_FAILED(rc)) {
-        vboxError(VIR_ERR_INTERNAL_ERROR,
-                  "%s", _("could not get children snapshots"));
-        goto cleanup;
-    }
-
-    if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) {
-        int top = children.count;
-        int next;
-
-        snapshots[0] = snap;
-        snap = NULL;
-        for (next = 0; next < count; next++) {
-            if (!snapshots[next])
-                break;
-            rc = vboxArrayGet(&children, snapshots[next],
-                              snapshots[next]->vtbl->GetChildren);
-            if (NS_FAILED(rc)) {
-                vboxError(VIR_ERR_INTERNAL_ERROR,
-                          "%s", _("could not get children snapshots"));
-                goto cleanup;
-            }
-            for (i = 0; i < children.count; i++) {
-                ISnapshot *child = children.items[i];
-                if (!child)
-                    continue;
-                if (top == count) {
-                    vboxError(VIR_ERR_INTERNAL_ERROR,
-                              _("unexpected number of snapshots > %u"), count);
-                    vboxArrayRelease(&children);
-                    goto cleanup;
-                }
-                VBOX_ADDREF(child);
-                snapshots[top++] = child;
-            }
-            vboxArrayRelease(&children);
-        }
-        count = top - 1;
-    } else {
-        count = children.count;
-    }
-
-    for (i = 0; i < nameslen; i++) {
-        PRUnichar *nameUtf16;
-        char *name;
-        int j = i + !!(flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS);
-
-        if (i >= count)
-            break;
-
-        rc = snapshots[j]->vtbl->GetName(snapshots[j], &nameUtf16);
-        if (NS_FAILED(rc) || !nameUtf16) {
-            vboxError(VIR_ERR_INTERNAL_ERROR,
-                      "%s", _("could not get snapshot name"));
-            goto cleanup;
-        }
-        VBOX_UTF16_TO_UTF8(nameUtf16, &name);
-        VBOX_UTF16_FREE(nameUtf16);
-        names[i] = strdup(name);
-        VBOX_UTF8_FREE(name);
-        if (!names[i]) {
-            virReportOOMError();
-            goto cleanup;
-        }
-    }
-
-    if (count <= nameslen)
-        ret = count;
-    else
-        ret = nameslen;
+    ret = vboxGetDescendantNames(data, snap, names, nameslen, recurse);
 
 cleanup:
-    if (count > 0) {
-        for (i = 0; i < count; i++)
-            VBOX_RELEASE(snapshots[i]);
-    }
-    VIR_FREE(snapshots);
     VBOX_RELEASE(machine);
     VBOX_RELEASE(snap);
     vboxIIDUnalloc(&iid);

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