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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list