[libvirt] [PATCH 0/3] Introduce new virtType enum item
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(-) -- 1.9.1
Introduce VIR_DOMAIN_VIRT_NONE to give domaintype the default value of zero. This is specially helpful in constructing better error messages when we don't want to look up the default emulator by virtType. The test data in vircapstest.c is also modified to reflect this change. --- src/conf/capabilities.c | 4 ++-- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + tests/vircapstest.c | 32 ++++++++++++++++---------------- 4 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 9c2c6b4..533c925 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -584,7 +584,7 @@ virCapsDomainDataCompare(virCapsGuestPtr guest, if ((arch != VIR_ARCH_NONE) && (guest->arch.id != arch)) return false; - if (domaintype != -1 && (!domain || domain->type != domaintype)) + if (domaintype != VIR_DOMAIN_VIRT_NONE && (!domain || domain->type != domaintype)) return false; if (emulator) { @@ -678,7 +678,7 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps, virDomainOSTypeToString(ostype)); if (arch) virBufferAsprintf(&buf, "arch=%s ", virArchToString(arch)); - if (domaintype != -1) + if (domaintype) virBufferAsprintf(&buf, "domaintype=%s ", virDomainVirtTypeToString(domaintype)); if (emulator) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6df1618..589546d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -106,6 +106,7 @@ VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, "custom-dtb"); VIR_ENUM_IMPL(virDomainVirt, VIR_DOMAIN_VIRT_LAST, + "none", "qemu", "kqemu", "kvm", @@ -14815,7 +14816,7 @@ virDomainDefParseXML(xmlDocPtr xml, virCapsDomainDataPtr capsdata = NULL; if (!(capsdata = virCapabilitiesDomainDataLookup(caps, def->os.type, - def->os.arch, use_virttype ? def->virtType : -1, + def->os.arch, use_virttype ? def->virtType : VIR_DOMAIN_VIRT_NONE, NULL, NULL))) goto error; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f043554..16f449d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -209,6 +209,7 @@ struct _virDomainDeviceDef { /* Different types of hypervisor */ /* NB: Keep in sync with virDomainVirtTypeToString impl */ typedef enum { + VIR_DOMAIN_VIRT_NONE = 0, VIR_DOMAIN_VIRT_QEMU, VIR_DOMAIN_VIRT_KQEMU, VIR_DOMAIN_VIRT_KVM, diff --git a/tests/vircapstest.c b/tests/vircapstest.c index 3b41654..acb0c03 100644 --- a/tests/vircapstest.c +++ b/tests/vircapstest.c @@ -223,39 +223,39 @@ test_virCapsDomainDataLookupQEMU(const void *data ATTRIBUTE_UNUSED) } /* Checking each parameter individually */ - CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, NULL, + CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-x86_64", "pc-0.11"); - CAPSCOMP(VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_NONE, -1, NULL, NULL, + CAPSCOMP(VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-x86_64", "pc-0.11"); - CAPSCOMP(-1, VIR_ARCH_AARCH64, -1, NULL, NULL, + CAPSCOMP(-1, VIR_ARCH_AARCH64, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_AARCH64, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-aarch64", "virt"); CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_KVM, NULL, NULL, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_X86_64, VIR_DOMAIN_VIRT_KVM, "/usr/bin/kvm", "pc"); - CAPSCOMP(-1, VIR_ARCH_NONE, -1, "/usr/bin/qemu-system-ppc64", NULL, + CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, "/usr/bin/qemu-system-ppc64", NULL, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_PPC64, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-ppc64", "pseries"); - CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, "s390-virtio", + CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, "s390-virtio", VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_S390X, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-s390x", "s390-virtio"); - CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, "pseries", + CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, "pseries", VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_PPC64, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-ppc64", "pseries"); - CAPSCOMP(-1, VIR_ARCH_PPC64LE, -1, NULL, "pseries", + CAPSCOMP(-1, VIR_ARCH_PPC64LE, VIR_DOMAIN_VIRT_NONE, NULL, "pseries", VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_PPC64LE, VIR_DOMAIN_VIRT_QEMU, "/usr/bin/qemu-system-ppc64", "pseries"); - CAPS_EXPECT_ERR(VIR_DOMAIN_OSTYPE_LINUX, VIR_ARCH_NONE, -1, NULL, NULL); - CAPS_EXPECT_ERR(-1, VIR_ARCH_PPC64LE, -1, NULL, "pc"); - CAPS_EXPECT_ERR(-1, VIR_ARCH_MIPS, -1, NULL, NULL); + CAPS_EXPECT_ERR(VIR_DOMAIN_OSTYPE_LINUX, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL); + CAPS_EXPECT_ERR(-1, VIR_ARCH_PPC64LE, VIR_DOMAIN_VIRT_NONE, NULL, "pc"); + CAPS_EXPECT_ERR(-1, VIR_ARCH_MIPS, VIR_DOMAIN_VIRT_NONE, NULL, NULL); CAPS_EXPECT_ERR(-1, VIR_ARCH_AARCH64, VIR_DOMAIN_VIRT_KVM, "/usr/bin/qemu-system-aarch64", NULL); - CAPS_EXPECT_ERR(-1, VIR_ARCH_NONE, -1, + CAPS_EXPECT_ERR(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, "/usr/bin/qemu-system-aarch64", "pc"); CAPS_EXPECT_ERR(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_VMWARE, NULL, "pc"); @@ -277,14 +277,14 @@ test_virCapsDomainDataLookupXen(const void *data ATTRIBUTE_UNUSED) goto out; } - CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, NULL, + CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_HVM, VIR_ARCH_I686, VIR_DOMAIN_VIRT_XEN, "/usr/lib/xen/bin/qemu-dm", "xenfv"); - CAPSCOMP(VIR_DOMAIN_OSTYPE_XEN, VIR_ARCH_NONE, -1, NULL, NULL, + CAPSCOMP(VIR_DOMAIN_OSTYPE_XEN, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_XEN, VIR_ARCH_I686, VIR_DOMAIN_VIRT_XEN, "/usr/lib/xen/bin/qemu-dm", "xenpv"); - CAPS_EXPECT_ERR(VIR_DOMAIN_OSTYPE_XEN, VIR_ARCH_NONE, -1, NULL, "xenfv"); + CAPS_EXPECT_ERR(VIR_DOMAIN_OSTYPE_XEN, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, "xenfv"); ret = 0; out: @@ -305,10 +305,10 @@ test_virCapsDomainDataLookupLXC(const void *data ATTRIBUTE_UNUSED) goto out; } - CAPSCOMP(-1, VIR_ARCH_NONE, -1, NULL, NULL, + CAPSCOMP(-1, VIR_ARCH_NONE, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_EXE, VIR_ARCH_X86_64, VIR_DOMAIN_VIRT_LXC, "/usr/libexec/libvirt_lxc", NULL); - CAPSCOMP(-1, VIR_ARCH_X86_64, -1, NULL, NULL, + CAPSCOMP(-1, VIR_ARCH_X86_64, VIR_DOMAIN_VIRT_NONE, NULL, NULL, VIR_DOMAIN_OSTYPE_EXE, VIR_ARCH_X86_64, VIR_DOMAIN_VIRT_LXC, "/usr/libexec/libvirt_lxc", NULL); -- 1.9.1
Earlier virtType was of type int. After, introducing the enum VIR_DOMAIN_VIRT_NONE, the type of virtType is modified to virDomainVirtType. --- src/conf/domain_conf.h | 2 +- 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 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 16f449d..5576e4d 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2189,7 +2189,7 @@ struct _virDomainKeyWrapDef { typedef struct _virDomainDef virDomainDef; typedef virDomainDef *virDomainDefPtr; struct _virDomainDef { - int virtType; + virDomainVirtType virtType; int id; unsigned char uuid[VIR_UUID_BUFLEN]; char *name; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 15ba39b..49d4aa2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1635,7 +1635,7 @@ qemuMonitorSetLink(qemuMonitorPtr mon, int qemuMonitorGetVirtType(qemuMonitorPtr mon, - int *virtType) + virDomainVirtType *virtType) { QEMU_CHECK_MONITOR(mon); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 720fb2d..2ce3958 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -360,7 +360,7 @@ int qemuMonitorSystemPowerdown(qemuMonitorPtr mon); int qemuMonitorGetCPUInfo(qemuMonitorPtr mon, int **pids); int qemuMonitorGetVirtType(qemuMonitorPtr mon, - int *virtType); + virDomainVirtType *virtType); int qemuMonitorGetBalloonInfo(qemuMonitorPtr mon, unsigned long long *currmem); int qemuMonitorGetMemoryStats(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 5c2f50f..df0c82a 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1339,7 +1339,7 @@ int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon, - int *virtType) + virDomainVirtType *virtType) { int ret; virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-kvm", diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index b76d85b..120bd93 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -56,7 +56,7 @@ int qemuMonitorJSONSystemReset(qemuMonitorPtr mon); int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon, int **pids); int qemuMonitorJSONGetVirtType(qemuMonitorPtr mon, - int *virtType); + virDomainVirtType *virtType); int qemuMonitorJSONUpdateVideoMemorySize(qemuMonitorPtr mon, virDomainVideoDefPtr video, char *path); diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index d5ef089..f44da20 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -564,7 +564,7 @@ int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, int qemuMonitorTextGetVirtType(qemuMonitorPtr mon, - int *virtType) + virDomainVirtType *virtType) { char *reply = NULL; diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index 3fa603b..53c503d 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -52,7 +52,7 @@ int qemuMonitorTextSystemReset(qemuMonitorPtr mon); int qemuMonitorTextGetCPUInfo(qemuMonitorPtr mon, int **pids); int qemuMonitorTextGetVirtType(qemuMonitorPtr mon, - int *virtType); + virDomainVirtType *virtType); int qemuMonitorTextGetBalloonInfo(qemuMonitorPtr mon, unsigned long long *currmem); int qemuMonitorTextGetMemoryStats(qemuMonitorPtr mon, diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index ded0423..17ebf64 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1311,7 +1311,7 @@ testQemuMonitorJSONqemuMonitorJSONGetVirtType(const void *data) virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); int ret = -1; - int virtType; + virDomainVirtType virtType; if (!test) return -1; -- 1.9.1
@domaintype: takes domain type to search for (of enum virDomainVirtType) since we no longer pass out-of-enum values. --- src/conf/capabilities.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 533c925..b420f9f 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -723,7 +723,7 @@ virCapabilitiesDomainDataLookupInternal(virCapsPtr caps, * @caps: capabilities to query * @ostype: guest operating system type, of enum VIR_DOMAIN_OSTYPE * @arch: Architecture to search for - * @domaintype: domain type to search for, of enum VIR_DOMAIN_VIRT + * @domaintype: domain type to search for, of enum virDomainVirtType * @emulator: Emulator path to search for * @machinetype: Machine type to search for * -- 1.9.1
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
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, 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 :|
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".
Thanks alot for your feedback. Should I modify and resend the patches after making the changes suggested above, if there are no other issues ? -- Regards, Shivangi Dhir
On 09/23/2015 12:51 AM, Shivangi Dhir wrote: [...]
Thanks alot for your feedback. Should I modify and resend the patches after making the changes suggested above, if there are no other issues ?
I should be able to jiggle your patches - I'll work on those tomorrow (for me). I also found out from practice that using virDomainVirtType in the capabilities.{h,c} functions & structures would result in many more changes in order to pull in the definition from domain_conf.h. So just kept them as int, but added a comment to make it more obvious. I'll end up with two patches, with the first getting the attached squashed in... Unless someone comes up with a path that's using the numerical representation - it just seemed the virtType would be (but I can check that). John
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(-)
I have pushed the changes upstream, congrats on your first libvirt patches. John
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Shivangi Dhir