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

[John Ferlan]

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

John