Re: [PATCH 2/2] snapshot: implement getparent for vbox

[Matthias Bolte <matthias.bolte@xxxxxxxxxxxxxx>]

2011/9/29 Eric Blake <eblake@xxxxxxxxxx>:
> Again, built by copying from existing functions.
>
> * src/vbox/vbox_tmpl.c (vboxDomainSnapshotGetParent): New function.
> ---
>
> I can only compile-test this; I'm relying on someone with an actual
> vbox setup to actually test it.

This patch needs some fixes, see below.

> Also, I didn't see anything in
> existing code that would efficiently implement
> virDomainSnapshotNumChildren; there may an API that I'm not aware of,
> but someone else will have to implement that API (Matthias?)

Is virDomainSnapshotNumChildren supposed to only return the number of
direct children, or all children (direct children, grandchildren, etc)
of a snapshot? In the later case they'll have to be counted
recursively, unfortunately.

>  src/vbox/vbox_tmpl.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 71 insertions(+), 0 deletions(-)
>
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index b483d0f..3d5f4ae 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -6046,6 +6046,76 @@ cleanup:
>  }
>
>  static virDomainSnapshotPtr
> +vboxDomainSnapshotGetParent(virDomainSnapshotPtr snapshot,
> +                            unsigned int flags)
> +{
> +    virDomainPtr dom = snapshot->domain;
> +    VBOX_OBJECT_CHECK(dom->conn, virDomainSnapshotPtr, NULL);
> +    vboxIID iid = VBOX_IID_INITIALIZER;
> +    IMachine *machine = NULL;
> +    ISnapshot *snap = NULL;
> +    ISnapshot *parent = NULL;
> +    PRUnichar *nameUtf16 = NULL;
> +    char *name = NULL;
> +    nsresult rc;
> +
> +    virCheckFlags(0, NULL);
> +
> +    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;

At this point you already have the snapshot you want.

> +    rc = machine->vtbl->GetCurrentSnapshot(machine, &snap);
> +    if (NS_FAILED(rc)) {
> +        vboxError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                  _("could not get current snapshot"));
> +        goto cleanup;
> +    }

This block here gives you the current snapshot, that's not what you want.

> +    rc = snap->vtbl->GetParent(snap, &parent);
> +    if (NS_FAILED(rc)) {
> +        vboxError(VIR_ERR_INTERNAL_ERROR,
> +                  _("could not get parent of snapshot %s"),
> +                  snapshot->name);
> +        goto cleanup;
> +    }
> +    if (!parent) {
> +        vboxError(VIR_ERR_NO_DOMAIN_SNAPSHOT,
> +                  _("snapshot '%s' does not have a parent"),
> +                  snapshot->name);
> +        goto cleanup;
> +    }
> +
> +    rc = parent->vtbl->GetName(parent, &nameUtf16);
> +    if (NS_FAILED(rc) || !nameUtf16) {
> +        vboxError(VIR_ERR_INTERNAL_ERROR,
> +                  _("could not get name of parent of snapshot %s"),
> +                  snapshot->name);
> +        goto cleanup;
> +    }
> +    VBOX_UTF16_TO_UTF8(nameUtf16, &name);
> +    VBOX_UTF16_FREE(nameUtf16);

Because VBOX_UTF16_FREE doesn't set the pointer to NULL, calling it
twice on the same pointer isn't safe.

> +
> +    ret = virGetDomainSnapshot(dom, name);
> +
> +cleanup:
> +    VBOX_UTF8_FREE(name);
> +    VBOX_UTF16_FREE(nameUtf16);

And this second call segfaults in the success path.

> +    VBOX_RELEASE(snap);
> +    VBOX_RELEASE(parent);
> +    VBOX_RELEASE(machine);
> +    vboxIIDUnalloc(&iid);
> +    return ret;
> +}

ACK with this diff applied:

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 3d5f4ae..2eb23fb 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -6072,13 +6072,6 @@
vboxDomainSnapshotGetParent(virDomainSnapshotPtr snapshot,
     if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name)))
         goto cleanup;

-    rc = machine->vtbl->GetCurrentSnapshot(machine, &snap);
-    if (NS_FAILED(rc)) {
-        vboxError(VIR_ERR_INTERNAL_ERROR, "%s",
-                  _("could not get current snapshot"));
-        goto cleanup;
-    }
-
     rc = snap->vtbl->GetParent(snap, &parent);
     if (NS_FAILED(rc)) {
         vboxError(VIR_ERR_INTERNAL_ERROR,
@@ -6101,7 +6094,10 @@
vboxDomainSnapshotGetParent(virDomainSnapshotPtr snapshot,
         goto cleanup;
     }
     VBOX_UTF16_TO_UTF8(nameUtf16, &name);
-    VBOX_UTF16_FREE(nameUtf16);
+    if (!name) {
+        virReportOOMError();
+        goto cleanup;
+    }

     ret = virGetDomainSnapshot(dom, name);


-- 
Matthias Bolte
http://photron.blogspot.com

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list