[libvirt] [PATCH 02/13] conf: Introduce helper to help getting correct def for getter functions

[Peter Krempa]

On Wed, Jun 17, 2015 at 16:53:14 -0400, John Ferlan wrote:
> 
> 
> On 06/15/2015 03:47 PM, Peter Krempa wrote:
> > virDomainObjGetOneDef will help to retrieve the correct definition
> > pointer from @vm in cases where VIR_DOMAIN_AFFECT_LIVE and
> > VIR_DOMAIN_AFFECT_CONFIG are mutually exclusive. The function simply
> > returns the correct pointer. This similarly to virDomainObjGetDefs will
> > greatly simplify the code.
> > ---
> >  src/conf/domain_conf.c   | 36 ++++++++++++++++++++++++++++++++++++
> >  src/conf/domain_conf.h   |  1 +
> >  src/libvirt_private.syms |  1 +
> >  3 files changed, 38 insertions(+)
> > 
> 
> I dunno - I just have this little voice asking is there some corner case
> from the prior virDomainLiveConfigHelperMethod thruough possibly
> creating 'newDef' in virDomainObjSetDefTransient that this new code will
> miss...  IOW - is there a condition where CONFIG is wanted, domain is
> running, but newDef is NULL.

Well, up until now I've inspected the qemu driver and the test driver
and both set the transient def upon start of a VM, so we should be fine
using this in those.

Honestly I think that all drivers should be fixed and
virDomainLiveConfigHelperMethod should be killed with fire forever.

> 
> OTOH - one wonders if the previous code went through the patch into
> virDomainObjSetDefTransient
> 
> All the callers check for NULL, so that's not an issue - just
> considering some strange corner case for a transient domain.
> 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 3cc182b..35e1cb4 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2948,6 +2948,42 @@ virDomainObjGetDefs(virDomainObjPtr vm,
> >  }
> > 
> > 
> > +/**
> > + * virDomainObjGetOneDef:
> > + *
> > + * @vm: Domain object
> > + * @flags: for virDomainModificationImpact
> > + *
> > + * Helper function to resolve @flags and return the correct domain pointer
> > + * object. This function returns one of @vm->def or @vm->persistentDef
> > + * according to @flags. This helper should be used only in APIs that guarantee
> > + * that @flags contains exactly one of VIR_DOMAIN_AFFECT_LIVE,
> 
> s/,/ or/
> 
> > + * VIR_DOMAIN_AFFECT_CONFIG.
> 
> and to be really redundant ;-) - couldn't resist.
> 
> s/./ and not both./
> 
> 
> ACK - although "GetOneDef" isn't my favorite, I don't have a better
> suggestion either.

Me neither but that can be changed in the future.

> 
> John

I've pushed this series with the suggested changes and I'll follow up
with fixes to the endjob label and the double space.

Thanks.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150618/580535aa/attachment-0001.sig>