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

[Pritesh Kothari]

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

thanks did it ;)

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

Did this.

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

No it also occurs when the machine is being restored from a saved state, but 
then again the machine was paused while saving it so I guess it is safe to 
consider it occurs only after unpausing the machine.

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

done.

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

the main problem here is that since we are in the callback of the machine 
which was just de-registered, we can't get back to virtualbox and ask for the 
name of the machine cause the machine is no more :( and thus the problem. i am 
trying to update this part so that the callback also gives the machine name, 
but it will take some time.


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

fixing this..

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

and this..

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

will surely try this, but not sure if it'd be easy, too many #if's :(

the patch with above changes is attached below:

Regards,
Pritesh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vbox_2009_07_17.patch
Type: text/x-patch
Size: 262497 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20090717/d9b532ea/attachment-0001.bin>