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@xxxxxxxxxx>
> 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.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list