Re: [libvirt] [PATCH 01/14] Add XML flag for internal domain status

On Mon, Jul 20, 2009 at 02:54:34PM +0100, Mark McLoughlin wrote: > On Mon, 2009-07-20 at 14:06 +0100, Daniel P. Berrange wrote: > > On Mon, Jul 20, 2009 at 02:18:38PM +0200, Daniel Veillard wrote: > > > > > Hum, that's very confusing. Why expose that flag at the API level > > > but forbid it's use from the API ? > > > Seems to me adding an extra parameter to the internal function > > > virDomainDefParseXML() is a far cleaner way to do things by looking at > > > this patch. > > > > It'd be nice to only have 1 flag parameter for the internal methods. > > Having 'flags' and 'privateFlags' to the same method is just going > > to lead to code errors, passing the wrong flag in and it silently > > failing > > > > We should not be adding this to the public API header file though. > > > > Since we only have 2 flags in use currently, lets just mask off > > the top 16 bits for internal use. > > > > So in domain_conf.h add a enum starting at the 16th bit > > > > > > typedef enum { > > VIR_DOMAIN_XML_INTERNAL_STATUS = (1<<16), /* dump internal domain status information */ > > } virDomainXMLInternalFlags; > > > > > > And to be sure no one abusing this from public API, in > > virDomainGetXMLDesc() scrub the incoming flags > > > > flags = flags & 0xffff; > > How's this? > > Cheers, > Mark. > > From: Mark McLoughlin <markmc(a)redhat.com&gt; > Subject: [PATCH 01/14] Add internal XML parsing/formatting flag > > We need to store things like device names and PCI slot numbers in the > qemu domain state file so that we don't lose that information on > libvirtd restart. Add a flag to indicate that this information should > be parsed or formatted. > > Make bit 16 and above of the flags bitmask for internal use only and > consume the first bit for this new status flag. > > * include/libvirt/libvirt.h: add VIR_DOMAIN_XML_FLAGS_MASK > > * src/libvirt.c: reject private flags in virDomainGetXMLDesc() > > * src/domain_conf.h: add VIR_DOMAIN_XML_INTERNAL_STATUS > > * src/domain_conf.c: pass the flag from virDomainObjParseXML() and > virDomainSaveStatus > --- > include/libvirt/libvirt.h | 5 +++-- > include/libvirt/libvirt.h.in | 5 +++-- > src/domain_conf.c | 8 ++++---- > src/domain_conf.h | 5 +++++ > src/libvirt.c | 6 ++++++ > 5 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/include/libvirt/libvirt.h b/include/libvirt/libvirt.h > index 90007a1..d1cf5fb 100644 > --- a/include/libvirt/libvirt.h > +++ b/include/libvirt/libvirt.h > @@ -584,8 +584,9 @@ int virDomainGetSecurityLabel (virDomainPtr domain, > */ > > typedef enum { > - VIR_DOMAIN_XML_SECURE = 1, /* dump security sensitive information too */ > - VIR_DOMAIN_XML_INACTIVE = 2/* dump inactive domain information */ > + VIR_DOMAIN_XML_SECURE = (1<<0), /* dump security sensitive information too */ > + VIR_DOMAIN_XML_INACTIVE = (1<<1), /* dump inactive domain information */ > + VIR_DOMAIN_XML_FLAGS_MASK = 0xffff, /* bits 16 and above are for internal use */ > } virDomainXMLFlags; IMHO, this should be kept in the private header file. The fact that we have hack reserving some portion for our internal use doesn't need to be exposed to applications.

> diff --git a/src/libvirt.c b/src/libvirt.c > index f4a7fa7..5507750 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -2730,6 +2730,12 @@ virDomainGetXMLDesc(virDomainPtr domain, int flags) > goto error; > } > > + if (flags != (flags & VIR_DOMAIN_XML_FLAGS_MASK)) { > + virLibConnError(conn, VIR_ERR_OPERATION_DENIED, > + _("virDomainGetXMLDesc with internal flags")); > + goto error; > + } Here I just think we should be masking the flags off silently. There's no need to explicit tell apps about the existance of internal flags.