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

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



On Mon, 2009-07-20 at 17:03 +0100, Daniel P. Berrange wrote:
> 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 redhat com>
> > 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.

Okay, I put it in libvirt_internal.h, assuming you don't want
domain_conf.h included in libvirt.c

> > 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.

Okay, done

Cheers,
Mark.


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