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

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

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