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

[Daniel P. Berrange]

On Fri, Jul 17, 2009 at 03:39:07PM +0200, Pritesh Kothari wrote:
> 
> > 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:

ACK, this looks way better with many of the #if's removed. We could
probably remove some more in a similar manner, but I think this is
OK to commit as is now.

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