[libvirt] [PATCH 02/13] conf: Introduce helper to help getting correct def for getter functions
Peter Krempa
pkrempa at redhat.com
Thu Jun 18 13:25:44 UTC 2015
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>
More information about the libvir-list
mailing list