On Mon, Jul 13, 2009 at 06:55:47PM +0200, Pritesh Kothari wrote:

Hi All,

I have added support for callbacks and VirtualBox version 3. I have attached the patch for the same, it is made against the head as of today.

To check if the callbacks work you can run the example from examples/domain- events/events-c directory and then start a domain to see if you get any callback events.

Regards, Pritesh

commit 1766765df5384cae45202c34ab20164d7c1544fe Author: pk221555 <pk221555@krishna.(none)> Date: Mon Jul 13 18:29:43 2009 +0200

I think your .gitconfig needs email addr fixing :-)

@@ -710,8 +863,13 @@ static virDomainPtr vboxDomainLookupByName(virConnectPtr conn, const char *name) IMachine **machines = NULL; PRUint32 machineCnt = 0; virDomainPtr dom = NULL; +#if VBOX_API_VERSION == 2002 nsID *iid = NULL; - char *machineName = NULL; +#else + PRUnichar *iidUtf16 = NULL; + char *iidUtf8 = NULL; +#endif + char *machineNameUtf8 = NULL; PRUnichar *machineNameUtf16 = NULL; unsigned char iidl[VIR_UUID_BUFLEN]; int i, matched = 0; @@ -735,16 +893,25 @@ static virDomainPtr vboxDomainLookupByName(virConnectPtr conn, const char *name) if (isAccessible) {

machine->vtbl->GetName(machine, &machineNameUtf16); - data->pFuncs->pfnUtf16ToUtf8(machineNameUtf16, &machineName); + data->pFuncs->pfnUtf16ToUtf8(machineNameUtf16, &machineNameUtf8);

- if (machineName && (STREQ(name, machineName))) { + if (STREQ(name, machineNameUtf8)) {

PRUint32 state;

matched = 1;

+#if VBOX_API_VERSION == 2002 machine->vtbl->GetId(machine, &iid); nsIDtoChar(iidl, iid); + data->pFuncs->pfnComUnallocMem(iid); +#else + machine->vtbl->GetId(machine, &iidUtf16); + data->pFuncs->pfnUtf16ToUtf8(iidUtf16, &iidUtf8); + virUUIDParse(iidUtf8, iidl); + data->pFuncs->pfnUtf8Free(iidUtf8); + data->pFuncs->pfnUtf16Free(iidUtf16); +#endif

[snip]

@@ -790,21 +961,34 @@ static int vboxDomainSuspend(virDomainPtr dom) { nsresult rc; vboxGlobalData *data = dom->conn->privateData; IMachine *machine = NULL; +#if VBOX_API_VERSION == 2002 nsID *iid = NULL; +#else + PRUnichar *iidUtf16 = NULL; + char iidUtf8[VIR_UUID_STRING_BUFLEN]; +#endif IConsole *console = NULL; PRUint32 state; int ret = -1;

+#if VBOX_API_VERSION == 2002 if (VIR_ALLOC(iid) < 0) { virReportOOMError(dom->conn); goto cleanup; } +#endif

if (data->vboxObj) { PRBool isAccessible = PR_FALSE;

+#if VBOX_API_VERSION == 2002 nsIDFromChar(iid, dom->uuid); rc = data->vboxObj->vtbl->GetMachine(data->vboxObj, iid, &machine); +#else + virUUIDFormat(dom->uuid, iidUtf8); + data->pFuncs->pfnUtf8ToUtf16(iidUtf8, &iidUtf16); + rc = data->vboxObj->vtbl->GetMachine(data->vboxObj, iidUtf16, &machine); +#endif if (NS_FAILED(rc)) { vboxError(dom->conn, VIR_ERR_INVALID_DOMAIN, "no domain with matching id %d", dom->id); @@ -820,7 +1004,11 @@ static int vboxDomainSuspend(virDomainPtr dom) {

if (state == MachineState_Running) { /* set state pause */ +#if VBOX_API_VERSION == 2002 data->vboxObj->vtbl->OpenExistingSession(data->vboxObj, data->vboxSession, iid); +#else + data->vboxObj->vtbl->OpenExistingSession(data->vboxObj, data->vboxSession, iidUtf16); +#endif data->vboxSession->vtbl->GetConsole(data->vboxSession, &console); if (console) { console->vtbl->Pause(console); @@ -844,7 +1032,11 @@ cleanup: if (machine) machine->vtbl->nsisupports.Release((nsISupports *)machine);

+#if VBOX_API_VERSION == 2002 VIR_FREE(iid); +#else + data->pFuncs->pfnUtf16Free(iidUtf16); +#endif return ret; }

I'm thinking it is going to get rather painful to #if/else/endif this stuff throughout the file. Perhaps it wouldbe better to define some wrapper datatypes / functions #if VBOX_API_VERSION == 2002 typedef vboxIID nsID; void vboxIIDFromDom(virDomainPtr dom, vboxIID *iid) { nsIDFromChar(iid, dom->uuid); } void vboxIIDFree(vboxIID *iid) { VIR_FREE(iid); } #else typedef vboxIID PRUnichar; void vboxIIDFromDom(virDomainPtr dom, vboxIID *iid) { char iidUtf8[VIR_UUID_STRING_BUFLEN]; virUUIDFormat(dom->uuid, iidUtf8); data->pFuncs->pfnUtf8ToUtf16(iidUtf8, iid); } void vboxIIDFree(vboxIID *iid) { data->pFuncs->pfnUtf16Free(iidUtf16); } #endif So, then the code could simply do vboxIIDFromDom(dom, &iid); rc = data->vboxObj->vtbl->GetMachine(data->vboxObj, iid, &machine); vboxIIDRelease(iid);

+#if VBOX_API_VERSION == 2002 + /* No Callback support for VirtualBox 2.2.* series */ +#else /* !(VBOX_API_VERSION == 2002) */ + +/* Functions needed for Callbacks */ +static nsresult vboxCallbackOnMachineStateChange (IVirtualBoxCallback *pThis, + PRUnichar * machineId, + PRUint32 state) { + virDomainPtr dom = NULL; + int event = 0; + int detail = 0; + + g_pVBoxGlobalData->domainEventDispatching = 1; + vboxDriverLock(g_pVBoxGlobalData); + + DEBUG("IVirtualBoxCallback: %p, State: %d", pThis, state); + DEBUGPRUnichar(machineId); + + if (machineId) { + char *machineIdUtf8 = NULL; + unsigned char uuid[VIR_UUID_BUFLEN]; + + g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(machineId, &machineIdUtf8); + virUUIDParse(machineIdUtf8, uuid); + + dom = vboxDomainLookupByUUID(g_pVBoxGlobalData->conn, uuid); + if (dom) { + int i = 0; + + if (state == MachineState_Starting) { + event = VIR_DOMAIN_EVENT_STARTED; + detail = VIR_DOMAIN_EVENT_STARTED_BOOTED; + } else if (state == MachineState_Restoring) { + event = VIR_DOMAIN_EVENT_STARTED; + detail = VIR_DOMAIN_EVENT_STARTED_RESTORED; + } else if (state == MachineState_Paused) { + event = VIR_DOMAIN_EVENT_SUSPENDED; + detail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED; + } else if (state == MachineState_Running) { + event = VIR_DOMAIN_EVENT_RESUMED; + detail = VIR_DOMAIN_EVENT_RESUMED_UNPAUSED;

Does 'MachineState_Running' only occur upon unpausing the VM ? The 'RESUMED' even is only intended to occur in that case.

+ } else if (state == MachineState_PoweredOff) { + event = VIR_DOMAIN_EVENT_STOPPED; + detail = VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN; + } else if (state == MachineState_Stopping) { + event = VIR_DOMAIN_EVENT_STOPPED; + detail = VIR_DOMAIN_EVENT_STOPPED_DESTROYED; + } else if (state == MachineState_Aborted) { + event = VIR_DOMAIN_EVENT_STOPPED; + detail = VIR_DOMAIN_EVENT_STOPPED_CRASHED; + } else if (state == MachineState_Saving) { + event = VIR_DOMAIN_EVENT_STOPPED; + detail = VIR_DOMAIN_EVENT_STOPPED_SAVED; + } else { + event = VIR_DOMAIN_EVENT_STOPPED; + detail = VIR_DOMAIN_EVENT_STOPPED_SHUTDOWN; + } +

+ +static nsresult vboxCallbackOnExtraDataCanChange (IVirtualBoxCallback *pThis, + PRUnichar * machineId, + PRUnichar * key, + PRUnichar * value, + PRUnichar * * error, + PRBool * allowChange) { + DEBUG("IVirtualBoxCallback: %p, allowChange: %s", pThis, *allowChange ? "true" : "false"); + DEBUGPRUnichar(machineId); + DEBUGPRUnichar(key); + DEBUGPRUnichar(value); + (void)error; /* so that the compiler doesn't complain about unsed variables */ + + return NS_OK; +}

Just add ATTRIBUTE_UNUSED to the parameter declaration instead of (void)error;

+ +static nsresult vboxCallbackOnMachineRegistered (IVirtualBoxCallback *pThis, + PRUnichar * machineId, + PRBool registered) { + virDomainPtr dom = NULL; + int event = 0; + int detail = 0; + + g_pVBoxGlobalData->domainEventDispatching = 1; + vboxDriverLock(g_pVBoxGlobalData); + + DEBUG("IVirtualBoxCallback: %p, registered: %s", pThis, registered ? "true" : "false"); + DEBUGPRUnichar(machineId); + + if (machineId) { + char *machineIdUtf8 = NULL; + unsigned char uuid[VIR_UUID_BUFLEN]; + + g_pVBoxGlobalData->pFuncs->pfnUtf16ToUtf8(machineId, &machineIdUtf8); + virUUIDParse(machineIdUtf8, uuid); + + dom = vboxDomainLookupByUUID(g_pVBoxGlobalData->conn, uuid); + if (dom) { + int i = 0; + + /* CURRENT LIMITATION: we never get the VIR_DOMAIN_EVENT_UNDEFINED + * event becuase the when the machine is de-registered the call + * to vboxDomainLookupByUUID fails and thus we don't get any + * dom pointer which is necessary (null dom pointer doesn't work) + * to show the VIR_DOMAIN_EVENT_UNDEFINED event + */

Hmm, that's a little annoying. You already have the UUID, and 'id' is obviously -1 for inactive guests. So only missing thing is the name :-(

+ +static nsresult vboxCallbackQueryInterface(nsISupports *pThis, const nsID *iid, void **resultp) { + IVirtualBoxCallback *that = (IVirtualBoxCallback *)pThis; + static const nsID ivirtualboxCallbackUUID = IVIRTUALBOXCALLBACK_IID; + static const nsID isupportIID = NS_ISUPPORTS_IID; + + /* Match UUID for IVirtualBoxCallback class */ + if ( memcmp(iid, &ivirtualboxCallbackUUID, sizeof(nsID)) == 0 + || memcmp(iid, &isupportIID, sizeof(nsID)) == 0) { + g_pVBoxGlobalData->vboxCallBackRefCount++; + *resultp = that; + + DEBUG("pThis: %p, vboxCallback QueryInterface: %d", pThis, g_pVBoxGlobalData->vboxCallBackRefCount); + + return NS_OK; + } + + + DEBUG("pThis: %p, vboxCallback QueryInterface didn't find a matching interface", pThis); + DEBUGUUID(iid); + DEBUGUUID(&ivirtualboxCallbackUUID); + return NS_NOINTERFACE; +} + + +static IVirtualBoxCallback *vboxAllocCallbackObj (virConnectPtr conn) { + IVirtualBoxCallback *vboxCallback = NULL; + + /* Allocate, Initialize and return a validi + * IVirtualBoxCallback object here + */ + if ((VIR_ALLOC(vboxCallback) < 0) || (VIR_ALLOC(vboxCallback->vtbl) < 0)) { + virReportOOMError(conn); + return NULL; + } + + { + vboxCallback->vtbl->nsisupports.AddRef = &vboxCallbackAddRef; + vboxCallback->vtbl->nsisupports.Release = &vboxCallbackRelease; + vboxCallback->vtbl->nsisupports.QueryInterface = &vboxCallbackQueryInterface; + vboxCallback->vtbl->OnMachineStateChange = &vboxCallbackOnMachineStateChange; + vboxCallback->vtbl->OnMachineDataChange = &vboxCallbackOnMachineDataChange; + vboxCallback->vtbl->OnExtraDataCanChange = &vboxCallbackOnExtraDataCanChange; + vboxCallback->vtbl->OnExtraDataChange = &vboxCallbackOnExtraDataChange; + vboxCallback->vtbl->OnMediaRegistered = &vboxCallbackOnMediaRegistered; + vboxCallback->vtbl->OnMachineRegistered = &vboxCallbackOnMachineRegistered; + vboxCallback->vtbl->OnSessionStateChange = &vboxCallbackOnSessionStateChange; + vboxCallback->vtbl->OnSnapshotTaken = &vboxCallbackOnSnapshotTaken; + vboxCallback->vtbl->OnSnapshotDiscarded = &vboxCallbackOnSnapshotDiscarded; + vboxCallback->vtbl->OnSnapshotChange = &vboxCallbackOnSnapshotChange; + vboxCallback->vtbl->OnGuestPropertyChange = &vboxCallbackOnGuestPropertyChange; + g_pVBoxGlobalData->vboxCallBackRefCount = 1; + + } + + return vboxCallback; +} + +static void vboxReadCallback(int watch ATTRIBUTE_UNUSED, + int fd, + int events ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) { + if (fd >= 0) { + g_pVBoxGlobalData->vboxQueue->vtbl->ProcessPendingEvents(g_pVBoxGlobalData->vboxQueue); + } else { + nsresult rc; + PLEvent *pEvent = NULL; + + rc = g_pVBoxGlobalData->vboxQueue->vtbl->WaitForEvent(g_pVBoxGlobalData->vboxQueue, &pEvent); + if (NS_SUCCEEDED(rc)) + g_pVBoxGlobalData->vboxQueue->vtbl->HandleEvent(g_pVBoxGlobalData->vboxQueue, pEvent); + } +} + +static int vboxDomainEventRegister (virConnectPtr conn, + virConnectDomainEventCallback callback, + void *opaque, + virFreeCallback freecb) { + nsresult rc; + vboxGlobalData *data = conn->privateData; + int vboxRet = -1; + int eventRet = -1; + int ret = -1; + + /* Locking has to be there as callbacks are not + * really fully thread safe + */ + vboxDriverLock(data); + + if(data->vboxObj) { + if (data->vboxCallback == NULL) { + data->vboxCallback = vboxAllocCallbackObj(conn); + if (data->vboxCallback != NULL) { + rc = data->vboxObj->vtbl->RegisterCallback(data->vboxObj, data->vboxCallback); + if (NS_SUCCEEDED(rc)) { + vboxRet = 0; + } + } + } else { + vboxRet = 0; + } + + /* Get the vbox file handle and add a event handle to it + * so that the events can be passed down to the user + */ + if (vboxRet == 0) { + PRInt32 vboxFileHandle; + vboxFileHandle = g_pVBoxGlobalData->vboxQueue->vtbl->GetEventQueueSelectFD(g_pVBoxGlobalData->vboxQueue); + + eventRet = virEventAddHandle(vboxFileHandle, VIR_EVENT_HANDLE_READABLE, vboxReadCallback, NULL, NULL); + + if (eventRet >= 0) {

You need to storage 'eventRet' in your virConnectPtr's privateData so that later...... I'm also surprised you can pass in 'NULL' to the 'opaque' parameter of virEventAddHandle(), because I'd expect your vboxReadCallback() function to need to have a reference to the your 'data' object or virConnectPtr object later.

+ /* Once a callback is registered with virtualbox, use a list + * to store the callbacks registered with libvirt so that + * later you can iterate over them + */ + + ret = virDomainEventCallbackListAdd(conn, data->domainEventCallbacks, + callback, opaque, freecb); + DEBUG("virDomainEventCallbackListAdd (ret = %d) ( conn: %p, " + "data->domainEventCallbacks: %p, callback: %p, opaque: %p, " + "freecb: %p )", ret, conn, data->domainEventCallbacks, callback, + opaque, freecb); + } + } + } + + vboxDriverUnlock(data); + + if (ret >= 0) { + return ret; + } else { + if (data->vboxObj && data->vboxCallback) { + data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); + } + return -1; + } +} + +static int vboxDomainEventDeregister (virConnectPtr conn, + virConnectDomainEventCallback callback) { + vboxGlobalData *data = conn->privateData; + int ret = -1; + + /* Locking has to be there as callbacks are not + * really fully thread safe + */ + vboxDriverLock(data); + + if (data->domainEventDispatching) + ret = virDomainEventCallbackListMarkDelete(conn, data->domainEventCallbacks, + callback); + else + ret = virDomainEventCallbackListRemove(conn, data->domainEventCallbacks, + callback); + + virEventRemoveHandle(0);

.....here you can pass a real handle ID, instead of '0' which will unregister some random callback that isn't neccessarily yours.

+ + if(data->vboxObj && data->vboxCallback) { + /* check count here of how many times register was called + * and only on the last de-register do the un-register call + */ + if (data->domainEventCallbacks && (data->domainEventCallbacks->count <= 0)) { + int i = 0; + + data->vboxObj->vtbl->UnregisterCallback(data->vboxObj, data->vboxCallback); + data->vboxCallback->vtbl->nsisupports.Release((nsISupports *)data->vboxCallback); + + /* iterate and free all the opaque objects using the + * freecb callback provided in vboxDomainEventRegister() + */ + for (i = 0; i < data->domainEventCallbacks->count; i++) { + if (data->domainEventCallbacks->callbacks[i]->freecb) { + data->domainEventCallbacks->callbacks[i]->freecb(data->domainEventCallbacks->callbacks[i]->opaque); + } + } + } + } + + vboxDriverUnlock(data); + + return ret; +}

I don't know how hard it'd be to unpick this now, but it'd be nice to have this in 2 patches, one adding support for version 3, and the 2nd then implementing the event callbacks. 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 :|