Re: [libvirt] [PATCH 1/1] Support for Callbacks and VirtualBox Version 3.

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

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);

> + 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.

> + (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;

> + > + /* 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 :-(

> + if (vboxRet == 0) { > + PRInt32 vboxFileHandle; > + vboxFileHandle = > g_pVBoxGlobalData->vboxQueue->vtbl->GetEventQueueSelectFD(g_pVBoxGlobalDa >ta->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.

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

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.