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 :|