5:47 a.m.
Unify detection of machine types and clean up some return value handling.
Ján Tomko (7):
Use qemuDomainMachineIs helpers when adding default devices
Invert condition in qemuDomainDefAddDefaultDevices
Return void in qemuDomainAssignARMVirtioMMIOAddresses
Remove useless variable in qemuDomainAssignAddresses
Rewrite the condition in qemuDomainAssignARMVirtioMMIOAddresses
Introduce qemuDomainMachineIsVirt
Remove useless os.machine NULL check
src/qemu/qemu_capabilities.c | 3 +--
src/qemu/qemu_command.c | 3 +--
src/qemu/qemu_domain.c | 32 +++++++++++++-----------------
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_domain_address.c | 45 ++++++++++++++++++++----------------------
5 files changed, 38 insertions(+), 46 deletions(-)
--
2.7.3
5:47 a.m.
New subject: [libvirt] [PATCH 1/7] Use qemuDomainMachineIs helpers when adding default devices
Do not duplicate the string comparisons by writing them twice.
---
src/qemu/qemu_domain.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 3f1fbd7..c17abbb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1740,8 +1740,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
addDefaultUSB = false;
break;
}
- if (STRPREFIX(def->os.machine, "pc-q35") ||
- STREQ(def->os.machine, "q35")) {
+ if (qemuDomainMachineIsQ35(def)) {
addPCIeRoot = true;
addImplicitSATA = true;
@@ -1754,11 +1753,7 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
addDefaultUSB = false;
break;
}
- if (!STRPREFIX(def->os.machine, "pc-0.") &&
- !STRPREFIX(def->os.machine, "pc-1.") &&
- !STRPREFIX(def->os.machine, "pc-i440") &&
- STRNEQ(def->os.machine, "pc") &&
- !STRPREFIX(def->os.machine, "rhel"))
+ if (!qemuDomainMachineIsI440FX(def))
break;
addPCIRoot = true;
break;
--
2.7.3
5:52 a.m.
New subject: [libvirt] [PATCH 1/7] Use qemuDomainMachineIs helpers when adding default devices
On Tue, May 03, 2016 at 12:47:21 +0200, Ján Tomko wrote:
Do not duplicate the string comparisons by writing them twice.
---
src/qemu/qemu_domain.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
ACK
5:47 a.m.
New subject: [libvirt] [PATCH 2/7] Invert condition in qemuDomainDefAddDefaultDevices
For all the other machine types, we use a positive condition.
Be more positive and use it for i440fx too.
---
src/qemu/qemu_domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c17abbb..9519334 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1753,9 +1753,8 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
addDefaultUSB = false;
break;
}
- if (!qemuDomainMachineIsI440FX(def))
- break;
- addPCIRoot = true;
+ if (qemuDomainMachineIsI440FX(def))
+ addPCIRoot = true;
break;
case VIR_ARCH_ARMV7L:
--
2.7.3
5:57 a.m.
New subject: [libvirt] [PATCH 2/7] Invert condition in qemuDomainDefAddDefaultDevices
On Tue, May 03, 2016 at 12:47:22 +0200, Ján Tomko wrote:
For all the other machine types, we use a positive condition.
Be more positive and use it for i440fx too.
---
src/qemu/qemu_domain.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
ACK
5:47 a.m.
New subject: [libvirt] [PATCH 3/7] Return void in qemuDomainAssignARMVirtioMMIOAddresses
This function does not fail and it does not need to return anything.
---
src/qemu/qemu_domain_address.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index c651dce..f149ff9 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -398,7 +398,7 @@ qemuDomainAssignS390Addresses(virDomainDefPtr def,
}
-static int
+static void
qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
{
@@ -411,7 +411,6 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
qemuDomainPrimeVirtioDeviceAddresses(
def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
}
- return 0;
}
@@ -1659,9 +1658,7 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
if (rc)
return rc;
- rc = qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps);
- if (rc)
- return rc;
+ qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps);
return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
}
--
2.7.3
5:58 a.m.
New subject: [libvirt] [PATCH 3/7] Return void in qemuDomainAssignARMVirtioMMIOAddresses
On Tue, May 03, 2016 at 12:47:23 +0200, Ján Tomko wrote:
This function does not fail and it does not need to return anything.
---
src/qemu/qemu_domain_address.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
ACK
5:47 a.m.
New subject: [libvirt] [PATCH 4/7] Remove useless variable in qemuDomainAssignAddresses
We do not need to propagate the exact return values
and the only possible ones are 0 and -1 anyway.
Remove the temporary variable and use the usual pattern:
if (f() < 0)
return -1;
---
src/qemu/qemu_domain_address.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index f149ff9..25aab39 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -1644,23 +1644,21 @@ qemuDomainAssignAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps,
virDomainObjPtr obj)
{
- int rc;
-
- rc = qemuDomainAssignVirtioSerialAddresses(def, obj);
- if (rc)
- return rc;
+ if (qemuDomainAssignVirtioSerialAddresses(def, obj) < 0)
+ return -1;
- rc = qemuDomainAssignSpaprVIOAddresses(def, qemuCaps);
- if (rc)
- return rc;
+ if (qemuDomainAssignSpaprVIOAddresses(def, qemuCaps) < 0)
+ return -1;
- rc = qemuDomainAssignS390Addresses(def, qemuCaps, obj);
- if (rc)
- return rc;
+ if (qemuDomainAssignS390Addresses(def, qemuCaps, obj) < 0)
+ return -1;
qemuDomainAssignARMVirtioMMIOAddresses(def, qemuCaps);
- return qemuDomainAssignPCIAddresses(def, qemuCaps, obj);
+ if (qemuDomainAssignPCIAddresses(def, qemuCaps, obj) < 0)
+ return -1;
+
+ return 0;
}
--
2.7.3
5:53 a.m.
New subject: [libvirt] [PATCH 4/7] Remove useless variable in qemuDomainAssignAddresses
On Tue, May 03, 2016 at 12:47:24 +0200, Ján Tomko wrote:
We do not need to propagate the exact return values
and the only possible ones are 0 and -1 anyway.
Remove the temporary variable and use the usual pattern:
if (f() < 0)
return -1;
---
src/qemu/qemu_domain_address.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
ACK
5:47 a.m.
New subject: [libvirt] [PATCH 5/7] Rewrite the condition in qemuDomainAssignARMVirtioMMIOAddresses
It was not indented correctly.
---
src/qemu/qemu_domain_address.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 25aab39..b3220a5 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -402,12 +402,16 @@ static void
qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
{
- if (((def->os.arch == VIR_ARCH_ARMV7L) ||
- (def->os.arch == VIR_ARCH_AARCH64)) &&
- (STRPREFIX(def->os.machine, "vexpress-") ||
- STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-")) &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
+ if (def->os.arch != VIR_ARCH_ARMV7L &&
+ def->os.arch != VIR_ARCH_AARCH64)
+ return;
+
+ if (!(STRPREFIX(def->os.machine, "vexpress-") ||
+ STREQ(def->os.machine, "virt") ||
+ STRPREFIX(def->os.machine, "virt-")))
+ return;
+
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
qemuDomainPrimeVirtioDeviceAddresses(
def, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO);
}
--
2.7.3
6 a.m.
New subject: [libvirt] [PATCH 5/7] Rewrite the condition in qemuDomainAssignARMVirtioMMIOAddresses
On Tue, May 03, 2016 at 12:47:25 +0200, Ján Tomko wrote:
It was not indented correctly.
---
src/qemu/qemu_domain_address.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
Also it was really hard to parse.
ACK
5:47 a.m.
New subject: [libvirt] [PATCH 6/7] Introduce qemuDomainMachineIsVirt
Use it everywhere except for virQEMUCapsFillDomainFeatureGICCaps.
---
src/qemu/qemu_capabilities.c | 3 +--
src/qemu/qemu_command.c | 3 +--
src/qemu/qemu_domain.c | 18 +++++++++++-------
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_domain_address.c | 6 ++----
5 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 677b06f..c081e84 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2184,8 +2184,7 @@ bool virQEMUCapsHasPCIMultiBus(virQEMUCapsPtr qemuCaps,
/* If 'virt' supports PCI, it supports multibus.
* No extra conditions here for simplicity.
*/
- if (STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-"))
+ if (qemuDomainMachineIsVirt(def))
return true;
}
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bd564db..bcc0e63 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6807,8 +6807,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
if (def->gic_version != VIR_GIC_VERSION_NONE) {
if ((def->os.arch != VIR_ARCH_ARMV7L &&
def->os.arch != VIR_ARCH_AARCH64) ||
- (STRNEQ(def->os.machine, "virt") &&
- !STRPREFIX(def->os.machine, "virt-"))) {
+ !qemuDomainMachineIsVirt(def)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("gic-version option is available "
"only for ARM virt machine"));
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9519334..53682c1 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1761,10 +1761,8 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
case VIR_ARCH_AARCH64:
addDefaultUSB = false;
addDefaultMemballoon = false;
- if (STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-")) {
+ if (qemuDomainMachineIsVirt(def))
addPCIeRoot = virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX);
- }
break;
case VIR_ARCH_PPC64:
@@ -1906,8 +1904,7 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def)
switch (def->os.arch) {
case VIR_ARCH_ARMV7L:
case VIR_ARCH_AARCH64:
- if (STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-")) {
+ if (qemuDomainMachineIsVirt(def)) {
/* GIC is always available to ARM virt machines */
def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
}
@@ -2039,8 +2036,7 @@ qemuDomainDefaultNetModel(const virDomainDef *def,
if (STREQ(def->os.machine, "versatilepb"))
return "smc91c111";
- if (STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-"))
+ if (qemuDomainMachineIsVirt(def))
return "virtio";
/* Incomplete. vexpress (and a few others) use this, but not all
@@ -4653,6 +4649,14 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def)
}
+bool
+qemuDomainMachineIsVirt(const virDomainDef *def)
+{
+ return STREQ(def->os.machine, "virt") ||
+ STRPREFIX(def->os.machine, "virt-");
+}
+
+
static bool
qemuCheckMemoryDimmConflict(const virDomainDef *def,
const virDomainMemoryDef *mem)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 63f98ba..95f821c 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -584,6 +584,7 @@ bool qemuDomainMachineIsQ35(const virDomainDef *def);
bool qemuDomainMachineIsI440FX(const virDomainDef *def);
bool qemuDomainMachineNeedsFDC(const virDomainDef *def);
bool qemuDomainMachineIsS390CCW(const virDomainDef *def);
+bool qemuDomainMachineIsVirt(const virDomainDef *def);
bool qemuDomainMachineHasBuiltinIDE(const virDomainDef *def);
int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver,
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index b3220a5..9c8c262 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -407,8 +407,7 @@ qemuDomainAssignARMVirtioMMIOAddresses(virDomainDefPtr def,
return;
if (!(STRPREFIX(def->os.machine, "vexpress-") ||
- STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-")))
+ qemuDomainMachineIsVirt(def)))
return;
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_MMIO)) {
@@ -1342,8 +1341,7 @@ qemuDomainSupportsPCI(virDomainDefPtr def,
if (STREQ(def->os.machine, "versatilepb"))
return true;
- if ((STREQ(def->os.machine, "virt") ||
- STRPREFIX(def->os.machine, "virt-")) &&
+ if (qemuDomainMachineIsVirt(def) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX))
return true;
--
2.7.3
6:05 a.m.
New subject: [libvirt] [PATCH 6/7] Introduce qemuDomainMachineIsVirt
On Tue, May 03, 2016 at 12:47:26 +0200, Ján Tomko wrote:
Use it everywhere except for virQEMUCapsFillDomainFeatureGICCaps.
---
src/qemu/qemu_capabilities.c | 3 +--
src/qemu/qemu_command.c | 3 +--
src/qemu/qemu_domain.c | 18 +++++++++++-------
src/qemu/qemu_domain.h | 1 +
src/qemu/qemu_domain_address.c | 6 ++----
5 files changed, 16 insertions(+), 15 deletions(-)
@@ -4653,6 +4649,14 @@ qemuDomainMachineIsS390CCW(const virDomainDef
*def)
}
+bool
+qemuDomainMachineIsVirt(const virDomainDef *def)
+{
+ return STREQ(def->os.machine, "virt") ||
+ STRPREFIX(def->os.machine, "virt-");
This could be transcribed as:
strcmp(def->os.machine, "virt") ||
strncmp(def->os.machine, "virt-", strlen("virt-")) so
"STRPREFIX(def->os.machine, "virt")" should be equivalent.
ACK regrardless.
6:19 a.m.
New subject: [libvirt] [PATCH 6/7] Introduce qemuDomainMachineIsVirt
On Tue, 2016-05-03 at 13:05 +0200, Peter Krempa wrote:
> +bool
> +qemuDomainMachineIsVirt(const virDomainDef *def)
> +{
> + return STREQ(def->os.machine, "virt") ||
> + STRPREFIX(def->os.machine, "virt-");
This could be transcribed as:
strcmp(def->os.machine, "virt") ||
strncmp(def->os.machine, "virt-", strlen("virt-")) so
"STRPREFIX(def->os.machine, "virt")" should be equivalent.
I think we want to make sure we only accept "virt" and
its versioned variants here - if QEMU introuced a new
machine type called "virtpc" we wouldn't want it to
pass the test.
(Then again, "virt-pc" would pass in any case. I think
the current check strikes a good balance, YMMV.)
--
Andrea Bolognani
Software Engineer - Virtualization Team
7:40 a.m.
New subject: [libvirt] [PATCH 6/7] Introduce qemuDomainMachineIsVirt
On Tue, 2016-05-03 at 13:19 +0200, Andrea Bolognani wrote:
On Tue, 2016-05-03 at 13:05 +0200, Peter Krempa wrote:
> >
> > +bool
> > +qemuDomainMachineIsVirt(const virDomainDef *def)
> > +{
> > + return STREQ(def->os.machine, "virt") ||
> > + STRPREFIX(def->os.machine, "virt-");
>
> This could be transcribed as:
> strcmp(def->os.machine, "virt") ||
> strncmp(def->os.machine, "virt-", strlen("virt-")) so
> "STRPREFIX(def->os.machine, "virt")" should be equivalent.
I think we want to make sure we only accept "virt" and
its versioned variants here - if QEMU introuced a new
machine type called "virtpc" we wouldn't want it to
pass the test.
(Then again, "virt-pc" would pass in any case. I think
the current check strikes a good balance, YMMV.)
How about something like the attached, untested patch? :)
--
Andrea Bolognani
Software Engineer - Virtualization Team
5:47 a.m.
New subject: [libvirt] [PATCH 7/7] Remove useless os.machine NULL check
In qemuDomainDefAddDefaultDevices we check for a non-NULL
def->os.machine for x86 archs, but not the others.
Moreover, the only caller - qemuDomainDefPostParse
already checks for it and even then it can happen only
if /etc/libvirt contains an XML without a machine type.
---
src/qemu/qemu_domain.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 53682c1..173f82c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1734,8 +1734,6 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
switch (def->os.arch) {
case VIR_ARCH_I686:
case VIR_ARCH_X86_64:
- if (!def->os.machine)
- break;
if (STREQ(def->os.machine, "isapc")) {
addDefaultUSB = false;
break;
--
2.7.3
6:06 a.m.
New subject: [libvirt] [PATCH 7/7] Remove useless os.machine NULL check
On Tue, May 03, 2016 at 12:47:27 +0200, Ján Tomko wrote:
In qemuDomainDefAddDefaultDevices we check for a non-NULL
def->os.machine for x86 archs, but not the others.
Moreover, the only caller - qemuDomainDefPostParse
already checks for it and even then it can happen only
if /etc/libvirt contains an XML without a machine type.
---
src/qemu/qemu_domain.c | 2 --
1 file changed, 2 deletions(-)
ACK
3161
days inactive
3161
days old
16 comments
3 participants
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Peter Krempa