Re: [libvirt] [PATCH 0/3] Introduce new virtType enum item

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

> > > > [0]

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