Hi,

On Tue, Sep 22, 2015 at 6:53 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
>
> On Tue, Sep 22, 2015 at 07:50:41AM -0400, John Ferlan wrote:
> >
> >
> > On 09/17/2015 04:46 AM, Shivangi Dhir wrote:
> > > These patches solve a BiteSizedTask - Introduce new virtType enum item[0]
> > >
> > > [0] http://wiki.libvirt.org/page/BiteSizedTasks#Introduce_new_virtType_enum_item
> > >
> > > Shivangi Dhir (3):
> > >   conf: Add new VIR_DOMAIN_VIRT_NONE enum
> > >   qemu: Make virtType of type virDomainVirtType
> > >   conf: Change the description of virCapabilitiesDomainDataLookup()
> > >
> > >  src/conf/capabilities.c      |  6 +++---
> > >  src/conf/domain_conf.c       |  3 ++-
> > >  src/conf/domain_conf.h       |  3 ++-
> > >  src/qemu/qemu_monitor.c      |  2 +-
> > >  src/qemu/qemu_monitor.h      |  2 +-
> > >  src/qemu/qemu_monitor_json.c |  2 +-
> > >  src/qemu/qemu_monitor_json.h |  2 +-
> > >  src/qemu/qemu_monitor_text.c |  2 +-
> > >  src/qemu/qemu_monitor_text.h |  2 +-
> > >  tests/qemumonitorjsontest.c  |  2 +-
> > >  tests/vircapstest.c          | 32 ++++++++++++++++----------------
> > >  11 files changed, 30 insertions(+), 28 deletions(-)
> > >
> >
> > While yes this is a bite sized task, I do have a concern with changing
> > the meaning of undefined from -1 to 0. Doing so will change (increase by
> > 1) each of the VIR_DOMAIN_VIRT_* values, e.g. VIR_DOMAIN_VIRT_QEMU
> > changes from 0 to 1. If you look through the history of changes to
> > virDomainVirtType you'll note it's been appended to and never had
> > something inserted in the middle.
> >
> > Inserting in the middle is not a problem if we could "guarantee" that
> > nothing exists (saved) or is currently running that references an
> > existing numerical value. Unfortunately, I'm not sure we can do that.
> >
> > As seen in patch 2, there is a 'virtType' value in the domain
> > definition. A running domain VIR_DOMAIN_VIRT_QEMU would have been
> > started with "0" in that field. When a new libvirtd with these changes
> > is in place, the running domain would still have "0" stored away and
> > that would mean something different.
> >
> > Furthermore (or likewise), a migration from an "older" host to a newer
> > host would have a different virtType value and I would think cause
> > virDomainDefCheckABIStability to complain. Similarly, a 'saved' domain
> > would have a numerical anomaly - although for that case it's less clear
> > whether we save the physical name or the numerical value.
> >
> > Of course I could be off-base/wrong, but let's see what others say...
>
> In general we should never be serializing the raw integer enum
> values only the string representation. If we did have somewhere
> that used an integer representation, we'd certainly have to avoid
> this change as you mention. I think we're probably safe though
> unless someone knows of a place we don't use the string rep.
>
> >
> > FWIW: For a "first" patch set - it seems you've followed the guidelines
> > quite well. About the only suggestions I have are in patch1 the
> > comparison "if (domaintype)" in virCapabilitiesDomainDataLookupInternal
> > could be "if (domaintype > VIR_DOMAIN_VIRT_NONE)" as well as patch3
> > should be merged with patch1 and the "int domaintype" in the args list
> > for virCapabilitiesDomainDataLookup (and capabilities.h) should be
> > "virDomainVirtType domaintype".
>

Regards,
Shivangi Dhir