[libvirt] [PATCH 0/9] qemu: Make qemu_domain more organized and consistent
A while ago I was looking at the code and got annoyed by the lack of consistency, both internal and external. This series addresses most of it. Andrea Bolognani (9): qemu: Remove redundant condition qemu: Use more specific prefixes qemu: Move functions around qemu: Add arch parameter to qemuDomainMachine*() qemu: Add arch checks to qemuDomainMachine*() qemu: Remove useless ARCH_IS_X86() call qemu: Make most qemuDomainMachine*() functions static qemu: Move qemuDomainSupportsPCI() to qemu_domain qemu: Unify style for qemuDomain*() src/qemu/qemu_capabilities.c | 10 +- src/qemu/qemu_domain.c | 250 +++++++++++++++++++++------------ src/qemu/qemu_domain.h | 29 ++-- src/qemu/qemu_domain_address.c | 22 --- 4 files changed, 175 insertions(+), 136 deletions(-) -- 2.20.1
No need to check whether we're dealing with a pSeries guest twice within just a few lines. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 81ef0357e7..7ccfcd054d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5224,8 +5224,10 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, VIR_DOMAIN_DISK_DEVICE_LUN); /* PowerPC pseries based VMs do not support floppy device */ - if (!qemuDomainMachineIsPSeries(machine, qemuCaps->arch)) + if (!qemuDomainMachineIsPSeries(machine, qemuCaps->arch)) { VIR_DOMAIN_CAPS_ENUM_SET(disk->diskDevice, VIR_DOMAIN_DISK_DEVICE_FLOPPY); + VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_FDC); + } if (qemuDomainMachineHasBuiltinIDE(machine)) VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_IDE); @@ -5235,10 +5237,6 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, VIR_DOMAIN_DISK_BUS_VIRTIO, /* VIR_DOMAIN_DISK_BUS_SD */); - /* PowerPC pseries based VMs do not support floppy device */ - if (!qemuDomainMachineIsPSeries(machine, qemuCaps->arch)) - VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_FDC); - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_USB_STORAGE)) VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_USB); -- 2.20.1
While the chances of the current checks resulting in false positives are basically zero, it's still nicer to check for the full prefix instead of the prefix's prefix. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b6c1a0e4e5..073f5cc86c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9880,7 +9880,7 @@ qemuDomainIsQ35(const virDomainDef *def) bool qemuDomainMachineIsQ35(const char *machine) { - return (STRPREFIX(machine, "pc-q35") || + return (STRPREFIX(machine, "pc-q35-") || STREQ(machine, "q35")); } @@ -9898,7 +9898,7 @@ qemuDomainMachineIsI440FX(const char *machine) return (STREQ(machine, "pc") || STRPREFIX(machine, "pc-0.") || STRPREFIX(machine, "pc-1.") || - STRPREFIX(machine, "pc-i440") || + STRPREFIX(machine, "pc-i440fx-") || STRPREFIX(machine, "rhel")); } -- 2.20.1
Make sure related functions, eg. all qemuDomainIs*(), are close together instead of being sprinkled throughout both the header and implementation file, and also that all qemuDomainMachine*() functions are declared first since we're going to make a bunch of them static later on. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 148 ++++++++++++++++++++--------------------- src/qemu/qemu_domain.h | 24 +++---- 2 files changed, 86 insertions(+), 86 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 073f5cc86c..bae49e97a4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9870,13 +9870,6 @@ qemuFindAgentConfig(virDomainDefPtr def) } -bool -qemuDomainIsQ35(const virDomainDef *def) -{ - return qemuDomainMachineIsQ35(def->os.machine); -} - - bool qemuDomainMachineIsQ35(const char *machine) { @@ -9885,13 +9878,6 @@ qemuDomainMachineIsQ35(const char *machine) } -bool -qemuDomainIsI440FX(const virDomainDef *def) -{ - return qemuDomainMachineIsI440FX(def->os.machine); -} - - bool qemuDomainMachineIsI440FX(const char *machine) { @@ -9904,14 +9890,23 @@ qemuDomainMachineIsI440FX(const char *machine) bool -qemuDomainHasPCIRoot(const virDomainDef *def) +qemuDomainMachineIsS390CCW(const char *machine) { - int root = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0); + return STRPREFIX(machine, "s390-ccw"); +} - if (root < 0) + +bool +qemuDomainMachineIsARMVirt(const char *machine, + const virArch arch) +{ + if (arch != VIR_ARCH_ARMV6L && + arch != VIR_ARCH_ARMV7L && + arch != VIR_ARCH_AARCH64) return false; - if (def->controllers[root]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) + if (STRNEQ(machine, "virt") && + !STRPREFIX(machine, "virt-")) return false; return true; @@ -9919,14 +9914,29 @@ qemuDomainHasPCIRoot(const virDomainDef *def) bool -qemuDomainHasPCIeRoot(const virDomainDef *def) +qemuDomainMachineIsRISCVVirt(const char *machine, + const virArch arch) { - int root = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0); + if (!ARCH_IS_RISCV(arch)) + return false; - if (root < 0) + if (STRNEQ(machine, "virt") && + !STRPREFIX(machine, "virt-")) return false; - if (def->controllers[root]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return true; +} + + +bool +qemuDomainMachineIsPSeries(const char *machine, + const virArch arch) +{ + if (!ARCH_IS_PPC64(arch)) + return false; + + if (STRNEQ(machine, "pseries") && + !STRPREFIX(machine, "pseries-")) return false; return true; @@ -9934,9 +9944,12 @@ qemuDomainHasPCIeRoot(const virDomainDef *def) bool -qemuDomainNeedsFDC(const virDomainDef *def) +qemuDomainMachineHasBuiltinIDE(const char *machine) { - return qemuDomainMachineNeedsFDC(def->os.machine); + return qemuDomainMachineIsI440FX(machine) || + STREQ(machine, "malta") || + STREQ(machine, "sun4u") || + STREQ(machine, "g3beige"); } @@ -9959,40 +9972,30 @@ qemuDomainMachineNeedsFDC(const char *machine) bool -qemuDomainIsS390CCW(const virDomainDef *def) +qemuDomainIsQ35(const virDomainDef *def) { - return qemuDomainMachineIsS390CCW(def->os.machine); + return qemuDomainMachineIsQ35(def->os.machine); } bool -qemuDomainMachineIsS390CCW(const char *machine) +qemuDomainIsI440FX(const virDomainDef *def) { - return STRPREFIX(machine, "s390-ccw"); + return qemuDomainMachineIsI440FX(def->os.machine); } bool -qemuDomainIsARMVirt(const virDomainDef *def) +qemuDomainIsS390CCW(const virDomainDef *def) { - return qemuDomainMachineIsARMVirt(def->os.machine, def->os.arch); + return qemuDomainMachineIsS390CCW(def->os.machine); } bool -qemuDomainMachineIsARMVirt(const char *machine, - const virArch arch) +qemuDomainIsARMVirt(const virDomainDef *def) { - if (arch != VIR_ARCH_ARMV6L && - arch != VIR_ARCH_ARMV7L && - arch != VIR_ARCH_AARCH64) - return false; - - if (STRNEQ(machine, "virt") && - !STRPREFIX(machine, "virt-")) - return false; - - return true; + return qemuDomainMachineIsARMVirt(def->os.machine, def->os.arch); } @@ -10004,14 +10007,21 @@ qemuDomainIsRISCVVirt(const virDomainDef *def) bool -qemuDomainMachineIsRISCVVirt(const char *machine, - const virArch arch) +qemuDomainIsPSeries(const virDomainDef *def) { - if (!ARCH_IS_RISCV(arch)) + return qemuDomainMachineIsPSeries(def->os.machine, def->os.arch); +} + + +bool +qemuDomainHasPCIRoot(const virDomainDef *def) +{ + int root = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0); + + if (root < 0) return false; - if (STRNEQ(machine, "virt") && - !STRPREFIX(machine, "virt-")) + if (def->controllers[root]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCI_ROOT) return false; return true; @@ -10019,24 +10029,31 @@ qemuDomainMachineIsRISCVVirt(const char *machine, bool -qemuDomainIsPSeries(const virDomainDef *def) +qemuDomainHasPCIeRoot(const virDomainDef *def) { - return qemuDomainMachineIsPSeries(def->os.machine, def->os.arch); + int root = virDomainControllerFind(def, VIR_DOMAIN_CONTROLLER_TYPE_PCI, 0); + + if (root < 0) + return false; + + if (def->controllers[root]->model != VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT) + return false; + + return true; } bool -qemuDomainMachineIsPSeries(const char *machine, - const virArch arch) +qemuDomainHasBuiltinIDE(const virDomainDef *def) { - if (!ARCH_IS_PPC64(arch)) - return false; + return qemuDomainMachineHasBuiltinIDE(def->os.machine); +} - if (STRNEQ(machine, "pseries") && - !STRPREFIX(machine, "pseries-")) - return false; - return true; +bool +qemuDomainNeedsFDC(const virDomainDef *def) +{ + return qemuDomainMachineNeedsFDC(def->os.machine); } @@ -10234,23 +10251,6 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, } -bool -qemuDomainHasBuiltinIDE(const virDomainDef *def) -{ - return qemuDomainMachineHasBuiltinIDE(def->os.machine); -} - - -bool -qemuDomainMachineHasBuiltinIDE(const char *machine) -{ - return qemuDomainMachineIsI440FX(machine) || - STREQ(machine, "malta") || - STREQ(machine, "sun4u") || - STREQ(machine, "g3beige"); -} - - /** * qemuDomainUpdateCurrentMemorySize: * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index fe474170dc..0bd4408f12 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -828,20 +828,8 @@ void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def); -bool qemuDomainIsQ35(const virDomainDef *def); -bool qemuDomainIsI440FX(const virDomainDef *def); -bool qemuDomainHasPCIRoot(const virDomainDef *def); -bool qemuDomainHasPCIeRoot(const virDomainDef *def); -bool qemuDomainNeedsFDC(const virDomainDef *def); -bool qemuDomainIsS390CCW(const virDomainDef *def); -bool qemuDomainIsARMVirt(const virDomainDef *def); -bool qemuDomainIsRISCVVirt(const virDomainDef *def); -bool qemuDomainIsPSeries(const virDomainDef *def); -bool qemuDomainHasBuiltinIDE(const virDomainDef *def); - bool qemuDomainMachineIsQ35(const char *machine); bool qemuDomainMachineIsI440FX(const char *machine); -bool qemuDomainMachineNeedsFDC(const char *machine); bool qemuDomainMachineIsS390CCW(const char *machine); bool qemuDomainMachineIsARMVirt(const char *machine, const virArch arch); @@ -850,6 +838,18 @@ bool qemuDomainMachineIsRISCVVirt(const char *machine, bool qemuDomainMachineIsPSeries(const char *machine, const virArch arch); bool qemuDomainMachineHasBuiltinIDE(const char *machine); +bool qemuDomainMachineNeedsFDC(const char *machine); + +bool qemuDomainIsQ35(const virDomainDef *def); +bool qemuDomainIsI440FX(const virDomainDef *def); +bool qemuDomainIsS390CCW(const virDomainDef *def); +bool qemuDomainIsARMVirt(const virDomainDef *def); +bool qemuDomainIsRISCVVirt(const virDomainDef *def); +bool qemuDomainIsPSeries(const virDomainDef *def); +bool qemuDomainHasPCIRoot(const virDomainDef *def); +bool qemuDomainHasPCIeRoot(const virDomainDef *def); +bool qemuDomainHasBuiltinIDE(const virDomainDef *def); +bool qemuDomainNeedsFDC(const virDomainDef *def); int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, virDomainObjPtr vm); -- 2.20.1
We want the signatures to be consistent, and also we're going to start using the additional parameter next. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_capabilities.c | 2 +- src/qemu/qemu_domain.c | 27 ++++++++++++++++----------- src/qemu/qemu_domain.h | 15 ++++++++++----- 3 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 7ccfcd054d..496fa94375 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5229,7 +5229,7 @@ virQEMUCapsFillDomainDeviceDiskCaps(virQEMUCapsPtr qemuCaps, VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_FDC); } - if (qemuDomainMachineHasBuiltinIDE(machine)) + if (qemuDomainMachineHasBuiltinIDE(machine, qemuCaps->arch)) VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, VIR_DOMAIN_DISK_BUS_IDE); VIR_DOMAIN_CAPS_ENUM_SET(disk->bus, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bae49e97a4..41996583fc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9871,7 +9871,8 @@ qemuFindAgentConfig(virDomainDefPtr def) bool -qemuDomainMachineIsQ35(const char *machine) +qemuDomainMachineIsQ35(const char *machine, + const virArch arch ATTRIBUTE_UNUSED) { return (STRPREFIX(machine, "pc-q35-") || STREQ(machine, "q35")); @@ -9879,7 +9880,8 @@ qemuDomainMachineIsQ35(const char *machine) bool -qemuDomainMachineIsI440FX(const char *machine) +qemuDomainMachineIsI440FX(const char *machine, + const virArch arch ATTRIBUTE_UNUSED) { return (STREQ(machine, "pc") || STRPREFIX(machine, "pc-0.") || @@ -9890,7 +9892,8 @@ qemuDomainMachineIsI440FX(const char *machine) bool -qemuDomainMachineIsS390CCW(const char *machine) +qemuDomainMachineIsS390CCW(const char *machine, + const virArch arch ATTRIBUTE_UNUSED) { return STRPREFIX(machine, "s390-ccw"); } @@ -9944,9 +9947,10 @@ qemuDomainMachineIsPSeries(const char *machine, bool -qemuDomainMachineHasBuiltinIDE(const char *machine) +qemuDomainMachineHasBuiltinIDE(const char *machine, + const virArch arch) { - return qemuDomainMachineIsI440FX(machine) || + return qemuDomainMachineIsI440FX(machine, arch) || STREQ(machine, "malta") || STREQ(machine, "sun4u") || STREQ(machine, "g3beige"); @@ -9954,7 +9958,8 @@ qemuDomainMachineHasBuiltinIDE(const char *machine) bool -qemuDomainMachineNeedsFDC(const char *machine) +qemuDomainMachineNeedsFDC(const char *machine, + const virArch arch ATTRIBUTE_UNUSED) { const char *p = STRSKIP(machine, "pc-q35-"); @@ -9974,21 +9979,21 @@ qemuDomainMachineNeedsFDC(const char *machine) bool qemuDomainIsQ35(const virDomainDef *def) { - return qemuDomainMachineIsQ35(def->os.machine); + return qemuDomainMachineIsQ35(def->os.machine, def->os.arch); } bool qemuDomainIsI440FX(const virDomainDef *def) { - return qemuDomainMachineIsI440FX(def->os.machine); + return qemuDomainMachineIsI440FX(def->os.machine, def->os.arch); } bool qemuDomainIsS390CCW(const virDomainDef *def) { - return qemuDomainMachineIsS390CCW(def->os.machine); + return qemuDomainMachineIsS390CCW(def->os.machine, def->os.arch); } @@ -10046,14 +10051,14 @@ qemuDomainHasPCIeRoot(const virDomainDef *def) bool qemuDomainHasBuiltinIDE(const virDomainDef *def) { - return qemuDomainMachineHasBuiltinIDE(def->os.machine); + return qemuDomainMachineHasBuiltinIDE(def->os.machine, def->os.arch); } bool qemuDomainNeedsFDC(const virDomainDef *def) { - return qemuDomainMachineNeedsFDC(def->os.machine); + return qemuDomainMachineNeedsFDC(def->os.machine, def->os.arch); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 0bd4408f12..e98999a026 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -828,17 +828,22 @@ void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def); -bool qemuDomainMachineIsQ35(const char *machine); -bool qemuDomainMachineIsI440FX(const char *machine); -bool qemuDomainMachineIsS390CCW(const char *machine); +bool qemuDomainMachineIsQ35(const char *machine, + const virArch arch); +bool qemuDomainMachineIsI440FX(const char *machine, + const virArch arch); +bool qemuDomainMachineIsS390CCW(const char *machine, + const virArch arch); bool qemuDomainMachineIsARMVirt(const char *machine, const virArch arch); bool qemuDomainMachineIsRISCVVirt(const char *machine, const virArch arch); bool qemuDomainMachineIsPSeries(const char *machine, const virArch arch); -bool qemuDomainMachineHasBuiltinIDE(const char *machine); -bool qemuDomainMachineNeedsFDC(const char *machine); +bool qemuDomainMachineHasBuiltinIDE(const char *machine, + const virArch arch); +bool qemuDomainMachineNeedsFDC(const char *machine, + const virArch arch); bool qemuDomainIsQ35(const virDomainDef *def); bool qemuDomainIsI440FX(const virDomainDef *def); -- 2.20.1
There is very little overlap in the machine types available on different architectures, so broadly speaking checking the machine type is usually enough; regardless, it's better to check the architecture as well. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 41996583fc..5feb743ad1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9872,8 +9872,11 @@ qemuFindAgentConfig(virDomainDefPtr def) bool qemuDomainMachineIsQ35(const char *machine, - const virArch arch ATTRIBUTE_UNUSED) + const virArch arch) { + if (!ARCH_IS_X86(arch)) + return false; + return (STRPREFIX(machine, "pc-q35-") || STREQ(machine, "q35")); } @@ -9881,8 +9884,11 @@ qemuDomainMachineIsQ35(const char *machine, bool qemuDomainMachineIsI440FX(const char *machine, - const virArch arch ATTRIBUTE_UNUSED) + const virArch arch) { + if (!ARCH_IS_X86(arch)) + return false; + return (STREQ(machine, "pc") || STRPREFIX(machine, "pc-0.") || STRPREFIX(machine, "pc-1.") || @@ -9893,8 +9899,11 @@ qemuDomainMachineIsI440FX(const char *machine, bool qemuDomainMachineIsS390CCW(const char *machine, - const virArch arch ATTRIBUTE_UNUSED) + const virArch arch) { + if (!ARCH_IS_S390(arch)) + return false; + return STRPREFIX(machine, "s390-ccw"); } @@ -9959,10 +9968,13 @@ qemuDomainMachineHasBuiltinIDE(const char *machine, bool qemuDomainMachineNeedsFDC(const char *machine, - const virArch arch ATTRIBUTE_UNUSED) + const virArch arch) { const char *p = STRSKIP(machine, "pc-q35-"); + if (!ARCH_IS_X86(arch)) + return false; + if (p) { if (STRPREFIX(p, "1.") || STREQ(p, "2.0") || -- 2.20.1
Now that we have added architecture checks to all qemuDomainIs*() functions, we no longer need to perform the same checks separately. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5feb743ad1..5820a4b515 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7775,7 +7775,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, * because other architectures and machine types were introduced * when libvirt already supported <controller type='usb'/>. */ - if (ARCH_IS_X86(def->os.arch) && qemuDomainIsI440FX(def) && + if (qemuDomainIsI440FX(def) && usb && usb->idx == 0 && (usb->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT || usb->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI) && -- 2.20.1
Ideally we'd make all of them static, but there are a few cases where we don't have a virDomainDef instance handy and so they are the only option. For the few ones we're forced to keep exporting, document through comments that the alternative is preferred. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 16 +++++++++++----- src/qemu/qemu_domain.h | 12 ++---------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5820a4b515..8db02f7738 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9870,7 +9870,7 @@ qemuFindAgentConfig(virDomainDefPtr def) } -bool +static bool qemuDomainMachineIsQ35(const char *machine, const virArch arch) { @@ -9882,7 +9882,7 @@ qemuDomainMachineIsQ35(const char *machine, } -bool +static bool qemuDomainMachineIsI440FX(const char *machine, const virArch arch) { @@ -9897,7 +9897,7 @@ qemuDomainMachineIsI440FX(const char *machine, } -bool +static bool qemuDomainMachineIsS390CCW(const char *machine, const virArch arch) { @@ -9908,6 +9908,8 @@ qemuDomainMachineIsS390CCW(const char *machine, } +/* You should normally avoid this function and use + * qemuDomainIsARMVirt() instead. */ bool qemuDomainMachineIsARMVirt(const char *machine, const virArch arch) @@ -9925,7 +9927,7 @@ qemuDomainMachineIsARMVirt(const char *machine, } -bool +static bool qemuDomainMachineIsRISCVVirt(const char *machine, const virArch arch) { @@ -9940,6 +9942,8 @@ qemuDomainMachineIsRISCVVirt(const char *machine, } +/* You should normally avoid this function and use + * qemuDomainIsPSeries() instead. */ bool qemuDomainMachineIsPSeries(const char *machine, const virArch arch) @@ -9955,6 +9959,8 @@ qemuDomainMachineIsPSeries(const char *machine, } +/* You should normally avoid this function and use + * qemuDomainHasBuiltinIDE() instead. */ bool qemuDomainMachineHasBuiltinIDE(const char *machine, const virArch arch) @@ -9966,7 +9972,7 @@ qemuDomainMachineHasBuiltinIDE(const char *machine, } -bool +static bool qemuDomainMachineNeedsFDC(const char *machine, const virArch arch) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index e98999a026..89f45aeb2c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -828,22 +828,14 @@ void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def); -bool qemuDomainMachineIsQ35(const char *machine, - const virArch arch); -bool qemuDomainMachineIsI440FX(const char *machine, - const virArch arch); -bool qemuDomainMachineIsS390CCW(const char *machine, - const virArch arch); +/* You should normally avoid these functions and use the variant that + * doesn't have "Machine" in the name instead. */ bool qemuDomainMachineIsARMVirt(const char *machine, const virArch arch); -bool qemuDomainMachineIsRISCVVirt(const char *machine, - const virArch arch); bool qemuDomainMachineIsPSeries(const char *machine, const virArch arch); bool qemuDomainMachineHasBuiltinIDE(const char *machine, const virArch arch); -bool qemuDomainMachineNeedsFDC(const char *machine, - const virArch arch); bool qemuDomainIsQ35(const virDomainDef *def); bool qemuDomainIsI440FX(const virDomainDef *def); -- 2.20.1
The function operates on a virDomainDef and is not tied to device address assignment in any way, so it makes more sense for it to live along with qemuDomainIs*() and the like. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 22 ++++++++++++++++++++++ src/qemu/qemu_domain.h | 2 ++ src/qemu/qemu_domain_address.c | 22 ---------------------- 3 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8db02f7738..f222f78654 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -10080,6 +10080,28 @@ qemuDomainNeedsFDC(const virDomainDef *def) } +bool +qemuDomainSupportsPCI(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps) +{ + if ((def->os.arch != VIR_ARCH_ARMV6L) && + (def->os.arch != VIR_ARCH_ARMV7L) && + (def->os.arch != VIR_ARCH_AARCH64) && + !ARCH_IS_RISCV(def->os.arch)) + return true; + + if (STREQ(def->os.machine, "versatilepb")) + return true; + + if ((qemuDomainIsARMVirt(def) || + qemuDomainIsRISCVVirt(def)) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX)) + return true; + + return false; +} + + static bool qemuCheckMemoryDimmConflict(const virDomainDef *def, const virDomainMemoryDef *mem) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 89f45aeb2c..f6b124e8fd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -847,6 +847,8 @@ bool qemuDomainHasPCIRoot(const virDomainDef *def); bool qemuDomainHasPCIeRoot(const virDomainDef *def); bool qemuDomainHasBuiltinIDE(const virDomainDef *def); bool qemuDomainNeedsFDC(const virDomainDef *def); +bool qemuDomainSupportsPCI(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps); int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, virDomainObjPtr vm); diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index c35ecd8585..32fdd59566 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -2356,28 +2356,6 @@ qemuDomainAssignDevicePCISlots(virDomainDefPtr def, } -static bool -qemuDomainSupportsPCI(virDomainDefPtr def, - virQEMUCapsPtr qemuCaps) -{ - if ((def->os.arch != VIR_ARCH_ARMV6L) && - (def->os.arch != VIR_ARCH_ARMV7L) && - (def->os.arch != VIR_ARCH_AARCH64) && - !ARCH_IS_RISCV(def->os.arch)) - return true; - - if (STREQ(def->os.machine, "versatilepb")) - return true; - - if ((qemuDomainIsARMVirt(def) || - qemuDomainIsRISCVVirt(def)) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX)) - return true; - - return false; -} - - static void qemuDomainPCIControllerSetDefaultModelName(virDomainControllerDefPtr cont, virDomainDefPtr def, -- 2.20.1
These functions do mostly the same things, and it would be preferrable if they did them in mostly the same ways. This also fixes a few violations to our code style guidelines. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 89 +++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f222f78654..eb4de1e6bd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9877,8 +9877,12 @@ qemuDomainMachineIsQ35(const char *machine, if (!ARCH_IS_X86(arch)) return false; - return (STRPREFIX(machine, "pc-q35-") || - STREQ(machine, "q35")); + if (STREQ(machine, "q35") || + STRPREFIX(machine, "pc-q35-")) { + return true; + } + + return false; } @@ -9889,11 +9893,15 @@ qemuDomainMachineIsI440FX(const char *machine, if (!ARCH_IS_X86(arch)) return false; - return (STREQ(machine, "pc") || - STRPREFIX(machine, "pc-0.") || - STRPREFIX(machine, "pc-1.") || - STRPREFIX(machine, "pc-i440fx-") || - STRPREFIX(machine, "rhel")); + if (STREQ(machine, "pc") || + STRPREFIX(machine, "pc-0.") || + STRPREFIX(machine, "pc-1.") || + STRPREFIX(machine, "pc-i440fx-") || + STRPREFIX(machine, "rhel")) { + return true; + } + + return false; } @@ -9904,7 +9912,10 @@ qemuDomainMachineIsS390CCW(const char *machine, if (!ARCH_IS_S390(arch)) return false; - return STRPREFIX(machine, "s390-ccw"); + if (STRPREFIX(machine, "s390-ccw")) + return true; + + return false; } @@ -9916,14 +9927,16 @@ qemuDomainMachineIsARMVirt(const char *machine, { if (arch != VIR_ARCH_ARMV6L && arch != VIR_ARCH_ARMV7L && - arch != VIR_ARCH_AARCH64) + arch != VIR_ARCH_AARCH64) { return false; + } - if (STRNEQ(machine, "virt") && - !STRPREFIX(machine, "virt-")) - return false; + if (STREQ(machine, "virt") || + STRPREFIX(machine, "virt-")) { + return true; + } - return true; + return false; } @@ -9934,11 +9947,12 @@ qemuDomainMachineIsRISCVVirt(const char *machine, if (!ARCH_IS_RISCV(arch)) return false; - if (STRNEQ(machine, "virt") && - !STRPREFIX(machine, "virt-")) - return false; + if (STREQ(machine, "virt") || + STRPREFIX(machine, "virt-")) { + return true; + } - return true; + return false; } @@ -9951,11 +9965,12 @@ qemuDomainMachineIsPSeries(const char *machine, if (!ARCH_IS_PPC64(arch)) return false; - if (STRNEQ(machine, "pseries") && - !STRPREFIX(machine, "pseries-")) - return false; + if (STREQ(machine, "pseries") || + STRPREFIX(machine, "pseries-")) { + return true; + } - return true; + return false; } @@ -9981,16 +9996,18 @@ qemuDomainMachineNeedsFDC(const char *machine, if (!ARCH_IS_X86(arch)) return false; - if (p) { - if (STRPREFIX(p, "1.") || - STREQ(p, "2.0") || - STREQ(p, "2.1") || - STREQ(p, "2.2") || - STREQ(p, "2.3")) - return false; - return true; + if (!p) + return false; + + if (STRPREFIX(p, "1.") || + STREQ(p, "2.0") || + STREQ(p, "2.1") || + STREQ(p, "2.2") || + STREQ(p, "2.3")) { + return false; } - return false; + + return true; } @@ -10084,19 +10101,21 @@ bool qemuDomainSupportsPCI(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) { - if ((def->os.arch != VIR_ARCH_ARMV6L) && - (def->os.arch != VIR_ARCH_ARMV7L) && - (def->os.arch != VIR_ARCH_AARCH64) && - !ARCH_IS_RISCV(def->os.arch)) + if (def->os.arch != VIR_ARCH_ARMV6L && + def->os.arch != VIR_ARCH_ARMV7L && + def->os.arch != VIR_ARCH_AARCH64 && + !ARCH_IS_RISCV(def->os.arch)) { return true; + } if (STREQ(def->os.machine, "versatilepb")) return true; if ((qemuDomainIsARMVirt(def) || qemuDomainIsRISCVVirt(def)) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX)) + virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_GPEX)) { return true; + } return false; } -- 2.20.1
On 2/6/19 2:46 PM, Andrea Bolognani wrote:
A while ago I was looking at the code and got annoyed by the lack of consistency, both internal and external. This series addresses most of it.
Andrea Bolognani (9): qemu: Remove redundant condition qemu: Use more specific prefixes qemu: Move functions around qemu: Add arch parameter to qemuDomainMachine*() qemu: Add arch checks to qemuDomainMachine*() qemu: Remove useless ARCH_IS_X86() call qemu: Make most qemuDomainMachine*() functions static qemu: Move qemuDomainSupportsPCI() to qemu_domain qemu: Unify style for qemuDomain*()
src/qemu/qemu_capabilities.c | 10 +- src/qemu/qemu_domain.c | 250 +++++++++++++++++++++------------ src/qemu/qemu_domain.h | 29 ++-- src/qemu/qemu_domain_address.c | 22 --- 4 files changed, 175 insertions(+), 136 deletions(-)
ACK Michal
participants (2)
-
Andrea Bolognani -
Michal Privoznik