[libvirt] [PATCH 0/7] qemu machine cleanups
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Peter Krempa