[libvirt] [PATCH 2/2] VirtualBox support (Resending by fixing a autodetection Bug)

[Daniel P. Berrange]

On Thu, Apr 09, 2009 at 11:25:08AM +0200, Pritesh Kothari wrote:
> Hi All,
> 
> Resending the second patch [PATCH 2/2] after fixing the Bug mentioned by 
> Florian on the list.
> 
> Regards,
> Pritesh
> 
> On Wednesday 08 April 2009 14:20:29 Pritesh Kothari wrote:
> > Hi All,
> >
> > I have attached a patch which when applied on the HEAD as of today would
> > allow virtualbox support in libvirt. It takes cares of all the stuff
> > mentioned on the list earlier. Still if I have missed anything, please do
> > tell me.
> >
> > The patch works very well with the VirtualBox OSE version and the 2.2
> > release.
> >
> > [PATCH 1/2] contains diff of files already in libvirt.
> > [PATCH 2/2] contains new files needed for VirtualBox support.



> diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c
> new file mode 100644
> index 0000000..8e0a771
> --- /dev/null
> +++ b/src/vbox/vbox_XPCOMCGlue.c
> @@ -0,0 +1,215 @@
> +/** @file vbox_XPCOMCGlue.c
> + * Glue code for dynamically linking to VBoxXPCOMC.
> + */
> +


> +/*******************************************************************************
> +*   Global Variables                                                           *
> +*******************************************************************************/
> +/** The dlopen handle for VBoxXPCOMC. */
> +void *g_hVBoxXPCOMC = NULL;
> +/** The last load error. */
> +char g_szVBoxErrMsg[256];
> +/** Pointer to the VBoxXPCOMC function table.  */
> +PCVBOXXPCOM g_pVBoxFuncs = NULL;
> +/** Pointer to VBoxGetXPCOMCFunctions for the loaded VBoxXPCOMC so/dylib/dll. */
> +PFNVBOXGETXPCOMCFUNCTIONS g_pfnGetFunctions = NULL;
> +/** boolean for checking if the VBOX_APP_HOME is already set by the users */
> +int g_bVAHSet = 0;

I don't much like the static fixed size error message buffer
here. It only appears to be used in one function:

> + * Try load VBoxXPCOMC.so/dylib/dll from the specified location and resolve all
> + * the symbols we need.
> + *
> + * @returns 0 on success, -1 on failure.
> + * @param   pszHome         The director where to try load VBoxXPCOMC from. Can be NULL.
> + */
> +static int tryLoadOne(const char *pszHome)
> +{
> +    size_t      cchHome = pszHome ? strlen(pszHome) : 0;
> +    size_t      cbReq;
> +    char        szBuf[4096];
> +    int         rc = -1;
> +
> +    /*
> +     * Construct the full name.
> +     */
> +    cbReq = cchHome + sizeof("/" DYNLIB_NAME);
> +    if (cbReq > sizeof(szBuf))
> +    {
> +        sprintf(g_szVBoxErrMsg, "path buffer too small: %u bytes required", (unsigned)cbReq);
> +        return -1;
> +    }
> +    memcpy(szBuf, pszHome, cchHome);
> +    szBuf[cchHome] = '/';
> +    cchHome++;
> +    memcpy(&szBuf[cchHome], DYNLIB_NAME, sizeof(DYNLIB_NAME));
> +
> +    /*
> +     * Try load it by that name, setting the VBOX_APP_HOME first (for now).
> +     * Then resolve and call the function table getter.
> +     */
> +    if (!g_bVAHSet)
> +    {
> +        /* Override it as we know that user didn't set it
> +         * and that we only did it in previous iteration
> +         */
> +        setenv("VBOX_APP_HOME", pszHome, 1);
> +    }
> +    g_hVBoxXPCOMC = dlopen(szBuf, RTLD_NOW | RTLD_LOCAL);
> +    if (g_hVBoxXPCOMC)
> +    {
> +        PFNVBOXGETXPCOMCFUNCTIONS pfnGetFunctions;
> +        pfnGetFunctions = (PFNVBOXGETXPCOMCFUNCTIONS)
> +            dlsym(g_hVBoxXPCOMC, VBOX_GET_XPCOMC_FUNCTIONS_SYMBOL_NAME);
> +        if (pfnGetFunctions)
> +        {
> +            g_pVBoxFuncs = pfnGetFunctions(VBOX_XPCOMC_VERSION);
> +            if (g_pVBoxFuncs)
> +            {
> +                g_pfnGetFunctions = pfnGetFunctions;
> +                rc = 0;
> +            }
> +            else
> +                sprintf(g_szVBoxErrMsg, "%.80s: pfnGetFunctions(%#x) failed",
> +                        szBuf, VBOX_XPCOMC_VERSION);
> +        }
> +        else
> +            sprintf(g_szVBoxErrMsg, "dlsym(%.80s/%.32s): %128s",
> +                    szBuf, VBOX_GET_XPCOMC_FUNCTIONS_SYMBOL_NAME, dlerror());
> +        if (rc != 0)
> +        {
> +            dlclose(g_hVBoxXPCOMC);
> +            g_hVBoxXPCOMC = NULL;
> +        }
> +    }
> +    else
> +        sprintf(g_szVBoxErrMsg, "dlopen(%.80s): %128s", szBuf, dlerror());
> +    return rc;
> +}

And the caller of this function just calls the formal libvirt error
reporting function via vboxError():


> +static int vboxInitialize(virConnectPtr conn, vboxGlobalData *data) {
> +
> +    if (VBoxCGlueInit() != 0) {
> +        vboxError(conn, VIR_ERR_INTERNAL_ERROR,"%s\n", g_szVBoxErrMsg);
> +        goto cleanup;
> +    }
> +
> +    if (g_pVBoxFuncs == NULL) {
> +        vboxError(conn, VIR_ERR_INTERNAL_ERROR,"%s\n", g_szVBoxErrMsg);
> +        goto cleanup;
> +    }
> +
> +    g_pVBoxFuncs->pfnComInitialize(&data->vboxObj, &data->vboxSession);
> +    if (data->vboxObj == NULL)
> +        goto cleanup;
> +    if (data->vboxSession == NULL)
> +        goto cleanup;
> +
> +    return 0;
> +
> +cleanup:
> +    return -1;
> +}

So, I think it'd be better to get rid of g_szVBoxErrMsg, and just have
tryLoadOne() call vboxError() (or virRaiseErrorHelper) directly.

> +
> +static virDrvOpenStatus vboxOpen(virConnectPtr conn,
> +                                 virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> +                                 int flags ATTRIBUTE_UNUSED) {
> +    vboxGlobalData *data;
> +    uid_t uid = getuid();
> +
> +    if (errorval == -1)
> +        return VIR_DRV_OPEN_DECLINED;
> +
> +    if (conn->uri == NULL) {
> +        conn->uri = xmlParseURI(uid ? "vbox:///session" : "vbox:///system");
> +        if (conn->uri == NULL) {
> +            vboxError(conn, VIR_ERR_NO_DOMAIN, NULL);
> +            return VIR_DRV_OPEN_ERROR;

Minor bug here, the VIR_ERR_NO_DOMAIN error isn't the correct code for an
URI parsing error :-)


Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|