[libvirt] [PATCHv2 11/7] snapshot: optimize vbox snapshot name lookup

[Matthias Bolte]

2011/10/4 Eric Blake <eblake at redhat.com>:
> Older VBox required grabbing all snapshots, then looking through
> them until a name match was found.  But when VBox 3.1 introduced
> snapshot branching, it also added the ability to lookup a snapshot
> by name instead of UUID; exploit this for faster snapshot lookup.
>
> * src/vbox/vbox_tmpl.c (vboxDomainSnapshotGet): Newer vbox added
> snapshot lookup by name.
> ---
>
> Caveat - this is written solely by reading VBox documentation, and
> I was only able to compile test.  I have no idea if this really
> works, or if VBox 3.1 is the right cut-off point.
>
>  src/vbox/vbox_tmpl.c |   31 ++++++++++++++++++++++++++++++-
>  1 files changed, 30 insertions(+), 1 deletions(-)
>
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 2eb23fb..ba2252c 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -5586,9 +5586,12 @@ vboxDomainSnapshotGet(vboxGlobalData *data,
>                       IMachine *machine,
>                       const char *name)
>  {
> -    ISnapshot **snapshots = NULL;
>     ISnapshot *snapshot = NULL;
>     nsresult rc;
> +
> +#if VBOX_API_VERSION < 3001
> +
> +    ISnapshot **snapshots = NULL;
>     int count = 0;
>     int i;
>
> @@ -5615,6 +5618,30 @@ vboxDomainSnapshotGet(vboxGlobalData *data,
>             break;
>     }
>
> +#else /* VBOX_API_VERSION >= 3001 */
> +    PRUnichar *nameUtf16 = NULL;
> +
> +    VBOX_UTF8_TO_UTF16(name, &nameUtf16);
> +    if (!nameUtf16) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +# if VBOX_API_VERSION < 4000
> +    rc = machine->vtbl->GetSnapshot(machine, nameUtf16, &snapshot);
> +# else /* VBOX_API_VERSION >= 4000 */
> +    rc = machine->vtbl->FindSnapshot(machine, nameUtf16, &snapshot);
> +# endif /* VBOX_API_VERSION >= 4000 */

GetSnapshot with a name is wrong. According to the docs GetSnapshot
takes an UUID and FindSnapshot takes a name. GetSnapshot also takes
NULL as UUID and returns the first snapshot then. GetSnapshot and
FindSnapshot have been there since version 2.2 and probably earlier.
In version 4.0 FindSnapshot and GetSnapshot have been merged into
FindSnapshot that now takes a UUID or a name.

So as you're doing a name lookup here GetSnapshot is wrong and
FindSnapshot has to be used independent from the API version.

I only tested this with VBox 4.0 and it worked, but it'll fail for
VBox < 4.0 I think, because of the use of GetSnapshot.

As FindSnapshot is available on all VBox versions this function should
probably use it unconditional and not fallback to
vboxDomainSnapshotGetAll for version < 3.1. But as stated I didn't
test this yet.

> +    VBOX_UTF16_FREE(nameUtf16);
> +    if (NS_FAILED(rc)) {
> +        vboxError(VIR_ERR_INTERNAL_ERROR,
> +                  _("could not get root snapshot for domain %s"),

This error message it wrong, in this branch you never tried to get the root.

> +                  dom->name);
> +        goto cleanup;
> +    }
> +
> +#endif /* VBOX_API_VERSION >= 3001 */
> +
>     if (!snapshot) {
>         vboxError(VIR_ERR_OPERATION_INVALID,

While at it, this error code is wrong, it should be VIR_ERR_NO_DOMAIN_SNAPSHOT.

Therefore, I suggest this diff on top of this patch to fix the
mentioned problems.

ACK, with this diff applied.

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 901799b..666d59d 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -5627,23 +5627,16 @@ vboxDomainSnapshotGet(vboxGlobalData *data,
         goto cleanup;
     }

-# if VBOX_API_VERSION < 4000
-    rc = machine->vtbl->GetSnapshot(machine, nameUtf16, &snapshot);
-# else /* VBOX_API_VERSION >= 4000 */
     rc = machine->vtbl->FindSnapshot(machine, nameUtf16, &snapshot);
-# endif /* VBOX_API_VERSION >= 4000 */
     VBOX_UTF16_FREE(nameUtf16);
     if (NS_FAILED(rc)) {
-        vboxError(VIR_ERR_INTERNAL_ERROR,
-                  _("could not get root snapshot for domain %s"),
-                  dom->name);
-        goto cleanup;
+        snapshot = NULL;
     }

 #endif /* VBOX_API_VERSION >= 3001 */

     if (!snapshot) {
-        vboxError(VIR_ERR_OPERATION_INVALID,
+        vboxError(VIR_ERR_NO_DOMAIN_SNAPSHOT,
                   _("domain %s has no snapshots with name %s"),
                   dom->name, name);
         goto cleanup;

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