3:46 a.m.
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
3:46 a.m.
New subject: [libvirt] [PATCH 1/3] conf: Add new VIR_DOMAIN_VIRT_NONE enum
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
3:46 a.m.
New subject: [libvirt] [PATCH 2/3] qemu: Make virtType of type virDomainVirtType
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
3:46 a.m.
New subject: [libvirt] [PATCH 3/3] conf: Change the description of virCapabilitiesDomainDataLookup()
@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
6:50 a.m.
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
8:23 a.m.
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 :|
11:51 p.m.
Hi,
On Tue, Sep 22, 2015 at 6:53 PM, Daniel P. Berrange <berrange(a)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
6:26 p.m.
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
2:45 p.m.
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
3312
days inactive
3320
days old
8 comments
3 participants
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Shivangi Dhir