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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list