[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/2] qemu: Turn closeCallbacks into virObjectLockable



On Tue, Feb 19, 2013 at 14:26:37 +0000, Daniel P. Berrange wrote:
> On Mon, Feb 18, 2013 at 05:07:44PM +0100, Jiri Denemark wrote:
> > To avoid having to hold the qemu driver lock while iterating through
> > close callbacks and calling them. This fixes a real deadlock when a
> > domain which is being migrated from another host gets autodestoyed as a
> > result of broken connection to the other host.
> 
> Since you're turning this into a full object, I think it would be
> nice to move it to a standalone file, so it can be reused by the
> LXC driver, which currently re-invents this same kind of code.

Good idea, could it be done by a follow-up patch? What do you think
would be the best name for such a shared object? I guess something like
virConnectCloseCallbacks could make sense...

> > -VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
> > +static int
> > +virQEMUCloseCallbacksOnceInit(void)
> > +{
> > +    virQEMUCloseCallbacksClass = virClassNew(virClassForObjectLockable(),
> > +                                             "virQEMUCloseCallbacks",
> > +                                             sizeof(virQEMUCloseCallbacks),
> > +                                             virQEMUCloseCallbacksDispose);
> > +    if (!virQEMUCloseCallbacksClass)
> > +        return -1;
> > +    return 0;
> > +}
> >  
> > +VIR_ONCE_GLOBAL_INIT(virQEMUConfig)
> > +VIR_ONCE_GLOBAL_INIT(virQEMUCloseCallbacks)
> 
> No need for two initializers, just make the virQEMUConfigOnceInit
> method create both classes.

OK, they will need to be separated again once this is decoupled from
qemu driver but that's fine.

> > -typedef virDomainObjPtr (*qemuDriverCloseCallback)(virQEMUDriverPtr driver,
> > -                                                   virDomainObjPtr vm,
> > -                                                   virConnectPtr conn);
> > -int qemuDriverCloseCallbackInit(virQEMUDriverPtr driver);
> > -void qemuDriverCloseCallbackShutdown(virQEMUDriverPtr driver);
> > -int qemuDriverCloseCallbackSet(virQEMUDriverPtr driver,
> > +typedef virDomainObjPtr (*virQEMUCloseCallback)(virQEMUDriverPtr driver,
> > +                                                virDomainObjPtr vm,
> > +                                                virConnectPtr conn);
> > +virQEMUCloseCallbacksPtr virQEMUCloseCallbacksNew(void);
> > +int virQEMUCloseCallbacksSet(virQEMUDriverPtr driver,
> 
> I was rather expecting to see the virQEMUDriverPtr replacd
> with a virQEMUCloseCallbacksPtr in all the methods now it
> becomes a full object.

I guess I was just lazy and didn't want to change callers too much :-) I
would need to do that when moving the code out of qemu driver anyway,
though.

> ACK to the general approach, but I think there's a few tweaks needed
> still.

OK, v2 is coming soon.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]