[libvirt] [PATCH v2 09/11] Give virDomainDef parser & formatter their own flags

[Daniel P. Berrange]

On Tue, Jan 13, 2015 at 03:05:48PM +0100, Jiri Denemark wrote:
> On Thu, Jan 08, 2015 at 15:48:20 +0000, Daniel Berrange wrote:
> > The virDomainDefParse* and virDomainDefFormat* methods both
> > accept the VIR_DOMAIN_XML_* flags defined in the public API,
> > along with a set of other VIR_DOMAIN_XML_INTERNAL_* flags
> > defined in domain_conf.c.
> > 
> > This is seriously confusing & error prone for a number of
> > reasons:
> > 
> >  - VIR_DOMAIN_XML_SECURE, VIR_DOMAIN_XML_MIGRATABLE and
> >    VIR_DOMAIN_XML_UPDATE_CPU are only relevant for the
> >    formatting operation
> >  - Some of the VIR_DOMAIN_XML_INTERNAL_* flags only apply
> >    to parse or to format, but not both.
> > 
> > This patch cleanly separates out the flags. There are two
> > distint VIR_DOMAIN_DEF_PARSE_* and VIR_DOMAIN_DEF_FORMAT_*
> > flags that are used by the corresponding methods. The
> > VIR_DOMAIN_XML_* flags received via public API calls must
> > be converted to the VIR_DOMAIN_DEF_FORMAT_* flags where
> > needed.
> > 
> > The various calls to virDomainDefParse which hardcoded the
> > use of the VIR_DOMAIN_XML_INACTIVE flag change to use the
> > VIR_DOMAIN_DEF_PARSE_INACTIVE flag.
> ...
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 1e3bede..4361834 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -86,39 +86,12 @@ struct _virDomainXMLOption {
> >  
> >      /* XML namespace callbacks */
> >      virDomainXMLNamespace ns;
> > - };
> > -
> > -
> > -/* Private flags used internally by virDomainSaveStatus and
> > - * virDomainLoadStatus, in addition to the public virDomainXMLFlags. */
> > -typedef enum {
> > -   /* dump internal domain status information */
> > -   VIR_DOMAIN_XML_INTERNAL_STATUS          = 1 << 16,
> > -   /* dump/parse <actual> element */
> > -   VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET      = 1 << 17,
> > -   /* dump/parse original states of host PCI device */
> > -   VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES = 1 << 18,
> > -   VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM       = 1 << 19,
> > -   VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT      = 1 << 20,
> > -   VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST    = 1 << 21,
> > -   /* parse only source half of <disk> */
> > -   VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE     = 1 << 22,
> > -} virDomainXMLInternalFlags;
> > -
> > -#define DUMPXML_FLAGS                           \
> > -    (VIR_DOMAIN_XML_SECURE |                    \
> > -     VIR_DOMAIN_XML_INACTIVE |                  \
> > -     VIR_DOMAIN_XML_UPDATE_CPU |                \
> > -     VIR_DOMAIN_XML_MIGRATABLE)
> > -
> > -verify(((VIR_DOMAIN_XML_INTERNAL_STATUS |
> > -         VIR_DOMAIN_XML_INTERNAL_ACTUAL_NET |
> > -         VIR_DOMAIN_XML_INTERNAL_PCI_ORIG_STATES |
> > -         VIR_DOMAIN_XML_INTERNAL_ALLOW_ROM |
> > -         VIR_DOMAIN_XML_INTERNAL_ALLOW_BOOT |
> > -         VIR_DOMAIN_XML_INTERNAL_CLOCK_ADJUST |
> > -         VIR_DOMAIN_XML_INTERNAL_DISK_SOURCE)
> > -        & DUMPXML_FLAGS) == 0);
> > +};
> > +
> > +#define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS             \
> > +    (VIR_DOMAIN_DEF_FORMAT_SECURE |                    \
> > +     VIR_DOMAIN_DEF_FORMAT_INACTIVE |                  \
> > +     VIR_DOMAIN_DEF_FORMAT_MIGRATABLE)
> 
> DUMPXML_FLAGS used to contain VIR_DOMAIN_XML_UPDATE_CPU so you either
> need to include it in VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS too or
> explicitly spell it everywhere VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS is
> used in place of DUMPXML_FLAGS.

So, when first looking at this I thought domain_conf.c did not need
the UPDATE_CPU flag at all, so I removed all this. Eventually I
found that it was quietly passed through to cpu_conf.c APIs so had
to re-add it, but I guess I forgot some bits. Same applies to all
the other places you mention UPDATE_CPU below. Will fix them all[


> > @@ -2160,7 +2162,8 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
> >  
> >      virUUIDFormat(vm->def->uuid, uuidstr);
> >      newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def,
> > -                                        QEMU_DOMAIN_FORMAT_LIVE_FLAGS, 1);
> > +                                        VIR_DOMAIN_DEF_FORMAT_SECURE |
> > +                                        VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU, 1);
> 
> What about
> virDomainDefFormatConvertXMLFlags(QEMU_DOMAIN_FORMAT_LIVE_FLAGS) to make
> sure we still use the right flags in case QEMU_DOMAIN_FORMAT_LIVE_FLAGS
> is ever changed?

QEMU_DOMAIN_FORMAT_LIVE_FLAGS uses the VIR_DOMAIN_XML flags from the
public API, whereas this method needs to use the internal flags.

> ACK with the small issues fixed (I don't want to review this for the
> second time).

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|