[libvirt] PATCH: Another attempt to fix vbox driver open

[Daniel Veillard]

On Fri, May 08, 2009 at 05:42:07PM +0100, Daniel P. Berrange wrote:
> The patches we just applied for the VirtualBox  open method still were 
> not quite right. It would return VIR_DRV_OPEN_DECLINED when uri==NULL,
> but before doing so it would have set conn->uri to vbox:///session. So
> even though it declined the connection, all the later drivers would now
> ignore it.  Also, it now returns DECLINED for some real errors that
> should be reported to the user.
> 
> Here's an alternative idea I've had for trying to address this. Some 
> goals:
> 
>  - If the user gives a URI with a vbox:///  prefix, we should always
>    handle it, unless a 'server' is set when we leave it to the remote
>    driver
>  - If an invalid path is given we must give back a real error code
>  - If after deciding the URI is for us, any initialization fails
>    we must raise an error.
>  - If the vbox glue layer is missing, we should still raise errors
>    for requested URIs, so user knows their URI is correct.

  All sounds good !

> To do this, I've taken the approach of registering a dummy vbox driver
> if the glue layer is missing. This just parses the URI and always returns
> an error for any vbox:// URIs that would otherwise work

  Interesting ...

[...]
> -    /* vboxRegister() shouldn't fail as that will render libvirt unless.
> -     * So, we use the v2.2 driver as a fallback/dummy.
> +    /*
> +     * If the glue layer won' initialize, we register a driver

   "won't" or "does not" :-)

> +     * with a dummy open method, so we can report nicer errors
> +     * if the user requests a vbox:// URI which we know won't
> +     * ever work

   "will never" ?

[...]
> +    if (conn->uri == NULL ||
> +        conn->uri->scheme == NULL ||
> +        STRNEQ (conn->uri->scheme, "vbox") ||
> +        conn->uri->server != NULL)
> +        return VIR_DRV_OPEN_DECLINED;

  Hum, we accept NULL to indicate Xen or KVM/QEmu by default.
Maybe if none of them is available, we should allow NULL to start
the VirtualBox driver. It could be useful say on MacOS, or
just if VirtualBox is installed on Linux while we know KVM is not.

> +    if (conn->uri->path == NULL || STREQ(conn->uri->path, "")) {
> +        vboxError(conn, VIR_ERR_INTERNAL_ERROR, "%s",
> +                  _("no VirtualBox drviver path specified (try vbox:///session)"));
> +        return VIR_DRV_OPEN_ERROR;
> +    }
[...]
> +static virDriver vboxDriverDummy = {
> +    VIR_DRV_VBOX,
> +    "VBOX",
> +    .open = vboxOpenDummy,
> +};

  In that case the new style initilization makes sense.


> @@ -291,13 +281,13 @@ static int vboxExtractVersion(virConnect
>  }
>  
>  static void vboxUninitialize(vboxGlobalData *data) {
> +    if (!data)
> +        return;
> +
>      if (data->pFuncs)
>          data->pFuncs->pfnComUninitialize();
>      VBoxCGlueTerm();
>  
> -    if (!data)
> -        return;
> -
>      virDomainObjListFree(&data->domains);
>      virCapabilitiesFree(data->caps);
>      VIR_FREE(data);

  That's a bug fix which should be applied in any case.

> @@ -306,52 +296,62 @@ static void vboxUninitialize(vboxGlobalD
>  static virDrvOpenStatus vboxOpen(virConnectPtr conn,
>                                   virConnectAuthPtr auth ATTRIBUTE_UNUSED,
>                                   int flags ATTRIBUTE_UNUSED) {
> -    vboxGlobalData *data;
> +    vboxGlobalData *data = NULL;
>      uid_t uid = getuid();
>  
>      if (conn->uri == NULL) {
>          conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system");

  Ah, okay, so NULL is still accepted, this is getting complex

If Pritesh can review and test the patch, I'm fine with it.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/