[libvirt] [PATCH 0/2] a few cleanups in qemu_domain_address.c
First patch is the standard "use VIR_AUTOFREE() in strings" type of patch. Second one is something I tried it out and ended up cleaning some repetitive code, so I believe it's worth seeing what you think. Daniel Henrique Barboza (2): qemu_domain_address.c: use VIR_AUTOFREE() in strings virpci, qemu_domain_address: adding virPCIDeviceSetAddress src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 126 +++++++++++++++------------------ src/util/virpci.c | 10 +++ src/util/virpci.h | 3 + 4 files changed, 70 insertions(+), 70 deletions(-) -- 2.21.0
A few 'cleanup' labels gone after using VIR_AUTOFREE() in the strings. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain_address.c | 69 +++++++++++++++------------------- 1 file changed, 30 insertions(+), 39 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index 216ba6235e..e20481607f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1524,12 +1524,11 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, * inappropriate address types. */ if (!info->pciConnectFlags) { - char *addrStr = virPCIDeviceAddressAsString(&info->addr.pci); + VIR_AUTOFREE(char *) addrStr = virPCIDeviceAddressAsString(&info->addr.pci); VIR_WARN("qemuDomainDeviceCalculatePCIConnectFlags() thinks that the " "device with PCI address %s should not have a PCI address", addrStr ? addrStr : "(unknown)"); - VIR_FREE(addrStr); info->pciConnectFlags = VIR_PCI_CONNECT_TYPE_PCI_DEVICE; } @@ -1720,11 +1719,10 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainPCIAddressSetPtr addrs) { - int ret = -1; size_t i; virPCIDeviceAddress tmp_addr; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); - char *addrStr = NULL; + VIR_AUTOFREE(char *) addrStr = NULL; virDomainPCIConnectFlags flags = (VIR_PCI_CONNECT_HOTPLUGGABLE | VIR_PCI_CONNECT_TYPE_PCI_DEVICE); @@ -1742,7 +1740,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, cont->info.addr.pci.function != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary IDE controller must have PCI address 0:0:1.1")); - goto cleanup; + return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -1762,7 +1760,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, cont->info.addr.pci.function != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PIIX3 USB controller at index 0 must have PCI address 0:0:1.2")); - goto cleanup; + return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -1777,7 +1775,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, } if (addrs->nbuses && virDomainPCIAddressReserveAddr(addrs, &cont->info.addr.pci, flags, 0) < 0) - goto cleanup; + return -1; } /* Implicit PIIX3 devices living on slot 1 not handled above */ @@ -1786,11 +1784,11 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, tmp_addr.slot = 1; /* ISA Bridge at 00:01.0 */ if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; /* Bridge at 00:01.3 */ tmp_addr.function = 3; if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; } if (def->nvideos > 0 && @@ -1808,26 +1806,26 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, tmp_addr.slot = 2; if (!(addrStr = virPCIDeviceAddressAsString(&tmp_addr))) - goto cleanup; + return -1; if (!virDomainPCIAddressValidate(addrs, &tmp_addr, addrStr, flags, true)) - goto cleanup; + return -1; if (virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { if (qemuDeviceVideoUsable) { if (qemuDomainPCIAddressReserveNextAddr(addrs, &primaryVideo->info) < 0) { - goto cleanup; + return -1; } } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI address 0:0:2.0 is in use, " "QEMU needs it for primary video")); - goto cleanup; + return -1; } } else { if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; primaryVideo->info.addr.pci = tmp_addr; primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } @@ -1838,7 +1836,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, primaryVideo->info.addr.pci.function != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary video card must have PCI address 0:0:2.0")); - goto cleanup; + return -1; } /* If TYPE == PCI, then qemuDomainCollectPCIAddress() function * has already reserved the address, so we must skip */ @@ -1852,13 +1850,10 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, " device will not be possible without manual" " intervention"); } else if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) { - goto cleanup; + return -1; } } - ret = 0; - cleanup: - VIR_FREE(addrStr); - return ret; + return 0; } @@ -1867,11 +1862,10 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, virQEMUCapsPtr qemuCaps, virDomainPCIAddressSetPtr addrs) { - int ret = -1; size_t i; virPCIDeviceAddress tmp_addr; bool qemuDeviceVideoUsable = virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY); - char *addrStr = NULL; + VIR_AUTOFREE(char *) addrStr = NULL; virDomainPCIConnectFlags flags = VIR_PCI_CONNECT_TYPE_PCIE_DEVICE; for (i = 0; i < def->ncontrollers; i++) { @@ -1891,7 +1885,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, cont->info.addr.pci.function != 2) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary SATA controller must have PCI address 0:0:1f.2")); - goto cleanup; + return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; @@ -1928,7 +1922,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, } if (assign) { if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; cont->info.addr.pci.domain = 0; @@ -1951,7 +1945,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, tmp_addr.slot = 0x1E; if (!virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; cont->info.addr.pci.domain = 0; @@ -1975,12 +1969,12 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, tmp_addr.function = 0; tmp_addr.multi = VIR_TRISTATE_SWITCH_ON; if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; tmp_addr.function = 3; tmp_addr.multi = VIR_TRISTATE_SWITCH_ABSENT; if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; } if (def->nvideos > 0 && @@ -1997,25 +1991,25 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, tmp_addr.slot = 1; if (!(addrStr = virPCIDeviceAddressAsString(&tmp_addr))) - goto cleanup; + return -1; if (!virDomainPCIAddressValidate(addrs, &tmp_addr, addrStr, flags, true)) - goto cleanup; + return -1; if (virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { if (qemuDeviceVideoUsable) { if (qemuDomainPCIAddressReserveNextAddr(addrs, &primaryVideo->info) < 0) - goto cleanup; + return -1; } else { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("PCI address 0:0:1.0 is in use, " "QEMU needs it for primary video")); - goto cleanup; + return -1; } } else { if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; primaryVideo->info.addr.pci = tmp_addr; } @@ -2026,7 +2020,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, primaryVideo->info.addr.pci.function != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary video card must have PCI address 0:0:1.0")); - goto cleanup; + return -1; } /* If TYPE == PCI, then qemuDomainCollectPCIAddress() function * has already reserved the address, so we must skip */ @@ -2041,7 +2035,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, " intervention"); virResetLastError(); } else if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) { - goto cleanup; + return -1; } } @@ -2062,7 +2056,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, continue; } if (virDomainPCIAddressReserveAddr(addrs, &tmp_addr, flags, 0) < 0) - goto cleanup; + return -1; sound->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; sound->info.addr.pci = tmp_addr; @@ -2070,10 +2064,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, } } - ret = 0; - cleanup: - VIR_FREE(addrStr); - return ret; + return 0; } -- 2.21.0
On Thu, Sep 19, 2019 at 05:04:46PM -0300, Daniel Henrique Barboza wrote:
A few 'cleanup' labels gone after using VIR_AUTOFREE() in the strings.
s/in the strings/on the @addrStr variable Reviewed-by: Erik Skultety <eskultet@redhat.com>
A common operation in qemu_domain_address is comparing a virPCIDeviceAddress and assigning domain, bus, slot and function to a specific value. The former can be done with the existing virPCIDeviceAddressEqual() helper. A new virpci helper called virPCIDeviceSetAddress() was created to make it easier to assign the same domain/bus/slot/function of an already existent virPCIDeviceAddress. Using both in qemu_domain_address made the code tidier, since the object was already created to use virPCIDeviceAddressEqual(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- I wasn't sure if this change was worth contributing it back, since it feels more like a 'look and feel' change. But since it made the code inside qemu_domain_address tidier, I decided to take a shot. src/libvirt_private.syms | 1 + src/qemu/qemu_domain_address.c | 57 ++++++++++++++++------------------ src/util/virpci.c | 10 ++++++ src/util/virpci.h | 3 ++ 4 files changed, 40 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 39812227aa..e878494d3e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2697,6 +2697,7 @@ virPCIDeviceNew; virPCIDeviceReattach; virPCIDeviceRebind; virPCIDeviceReset; +virPCIDeviceSetAddress; virPCIDeviceSetManaged; virPCIDeviceSetRemoveSlot; virPCIDeviceSetReprobe; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index e20481607f..8eb1f0656f 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; + virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0, + .slot = 1, .function = 1}; + virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0, + .slot = 1, .function = 2}; /* First IDE controller lives on the PIIX3 at slot=1, function=1 */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0) { if (virDeviceInfoPCIAddressIsPresent(&cont->info)) { - if (cont->info.addr.pci.domain != 0 || - cont->info.addr.pci.bus != 0 || - cont->info.addr.pci.slot != 1 || - cont->info.addr.pci.function != 1) { + if (!virPCIDeviceAddressEqual(&cont->info.addr.pci, + &primaryIDEAddr)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary IDE controller must have PCI address 0:0:1.1")); + _("Primary IDE controller must have PCI " + "address 0:0:1.1")); return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - cont->info.addr.pci.domain = 0; - cont->info.addr.pci.bus = 0; - cont->info.addr.pci.slot = 1; - cont->info.addr.pci.function = 1; + virPCIDeviceSetAddress(&cont->info.addr.pci, &primaryIDEAddr); } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->idx == 0 && (cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI || cont->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_DEFAULT)) { if (virDeviceInfoPCIAddressIsPresent(&cont->info)) { - if (cont->info.addr.pci.domain != 0 || - cont->info.addr.pci.bus != 0 || - cont->info.addr.pci.slot != 1 || - cont->info.addr.pci.function != 2) { + if (!virPCIDeviceAddressEqual(&cont->info.addr.pci, + &piix3USBAddr)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("PIIX3 USB controller at index 0 must have PCI address 0:0:1.2")); + _("PIIX3 USB controller at index 0 must " + "have PCI address 0:0:1.2")); return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - cont->info.addr.pci.domain = 0; - cont->info.addr.pci.bus = 0; - cont->info.addr.pci.slot = 1; - cont->info.addr.pci.function = 2; + virPCIDeviceSetAddress(&cont->info.addr.pci, &piix3USBAddr); } } else { /* this controller is not skipped in qemuDomainCollectPCIAddress */ @@ -1800,6 +1796,8 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, * at slot 2. */ virDomainVideoDefPtr primaryVideo = def->videos[0]; + virPCIDeviceAddress primaryCardAddr = {.domain = 0, .bus = 0, + .slot = 2, .function = 0}; if (virDeviceInfoPCIAddressIsWanted(&primaryVideo->info)) { memset(&tmp_addr, 0, sizeof(tmp_addr)); @@ -1830,10 +1828,8 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, primaryVideo->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; } } else if (!qemuDeviceVideoUsable) { - if (primaryVideo->info.addr.pci.domain != 0 || - primaryVideo->info.addr.pci.bus != 0 || - primaryVideo->info.addr.pci.slot != 2 || - primaryVideo->info.addr.pci.function != 0) { + if (!virPCIDeviceAddressEqual(&primaryVideo->info.addr.pci, + &primaryCardAddr)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Primary video card must have PCI address 0:0:2.0")); return -1; @@ -1870,6 +1866,8 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; + virPCIDeviceAddress primarySATAAddr = {.domain = 0, .bus = 0, + .slot = 0x1F, .function = 2}; switch (cont->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SATA: @@ -1879,20 +1877,17 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, */ if (cont->idx == 0) { if (virDeviceInfoPCIAddressIsPresent(&cont->info)) { - if (cont->info.addr.pci.domain != 0 || - cont->info.addr.pci.bus != 0 || - cont->info.addr.pci.slot != 0x1F || - cont->info.addr.pci.function != 2) { + if (!virPCIDeviceAddressEqual(&cont->info.addr.pci, + &primarySATAAddr)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary SATA controller must have PCI address 0:0:1f.2")); + _("Primary SATA controller must have " + "PCI address 0:0:1f.2")); return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - cont->info.addr.pci.domain = 0; - cont->info.addr.pci.bus = 0; - cont->info.addr.pci.slot = 0x1F; - cont->info.addr.pci.function = 2; + virPCIDeviceSetAddress(&cont->info.addr.pci, + &primarySATAAddr); } } break; diff --git a/src/util/virpci.c b/src/util/virpci.c index ee78151e74..12ec05ab9e 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1483,6 +1483,16 @@ virPCIDeviceGetName(virPCIDevicePtr dev) return dev->name; } +void +virPCIDeviceSetAddress(virPCIDeviceAddress *addr1, + const virPCIDeviceAddress *addr2) +{ + addr1->domain = addr2->domain; + addr1->bus = addr2->bus; + addr1->slot = addr2->slot; + addr1->function = addr2->function; +} + /** * virPCIDeviceGetConfigPath: * diff --git a/src/util/virpci.h b/src/util/virpci.h index dc20f91710..b192e1cf19 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -234,6 +234,9 @@ bool virPCIDeviceAddressIsEmpty(const virPCIDeviceAddress *addr); bool virPCIDeviceAddressEqual(const virPCIDeviceAddress *addr1, const virPCIDeviceAddress *addr2); +void virPCIDeviceSetAddress(virPCIDeviceAddress *addr1, + const virPCIDeviceAddress *addr2); + char *virPCIDeviceAddressAsString(const virPCIDeviceAddress *addr) ATTRIBUTE_NONNULL(1); -- 2.21.0
On Thu, Sep 19, 2019 at 05:04:47PM -0300, Daniel Henrique Barboza wrote:
A common operation in qemu_domain_address is comparing a virPCIDeviceAddress and assigning domain, bus, slot and function to a specific value. The former can be done with the existing virPCIDeviceAddressEqual() helper.
A new virpci helper called virPCIDeviceSetAddress() was created to make it easier to assign the same domain/bus/slot/function of an already existent virPCIDeviceAddress. Using both in qemu_domain_address made the code tidier, since the object was already created to use virPCIDeviceAddressEqual().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> ---
I wasn't sure if this change was worth contributing it back, since it feels more like a 'look and feel' change. But since it made the code inside qemu_domain_address tidier, I decided to take a shot.
I actually think it does enhance readability, so why not. However... [...]
--- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; + virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0, + .slot = 1, .function = 1}; + virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0, + .slot = 1, .function = 2};
/* First IDE controller lives on the PIIX3 at slot=1, function=1 */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0) { if (virDeviceInfoPCIAddressIsPresent(&cont->info)) { - if (cont->info.addr.pci.domain != 0 || - cont->info.addr.pci.bus != 0 || - cont->info.addr.pci.slot != 1 || - cont->info.addr.pci.function != 1) { + if (!virPCIDeviceAddressEqual(&cont->info.addr.pci, + &primaryIDEAddr)) {
I think this virPCIDeviceAddressEqual consolidation might be a separate patch (matter of taste, feel free to disagree, but to me the changes are not strictly related and can be easily split).
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary IDE controller must have PCI address 0:0:1.1")); + _("Primary IDE controller must have PCI " + "address 0:0:1.1"));
Unrelated...but it can stay...
return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - cont->info.addr.pci.domain = 0; - cont->info.addr.pci.bus = 0; - cont->info.addr.pci.slot = 1; - cont->info.addr.pci.function = 1; + virPCIDeviceSetAddress(&cont->info.addr.pci, &primaryIDEAddr);
So, given the signature, it feels more like a Copy rather than Set. I imagine a setter to enumerate the scalar types and the destination where the values should end up in, in this regard, it's not a proper setter because the struct has a few more elements that technically can be set, but if you created such a setter I'd say we didn't gain anything in terms of readability at all, especially when you could do: /* provided that I'm interpreting 6.7.8.19 [1] of the C99 standard correctly, * everything else we're not using should be 0, otherwise we can't do this. * Alternatively, you'd need to reset the other members as well */ cont->info.addr.pci = primaryIDEAddr; I'm not completely sold on the helper the way it is now, but I'm up for the idea of the patch. Creating a proper Copy helper would bring the obvious issue of needing to retain the original settings of .multi and zPCI to make it universal, that's why I'm opting for the direct assignment, because all of the relevant code in the patch should be x86 only and multifunction would be reset to 0, which is also the default setting anyway, so that should play nicely as well. [1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf Erik
On 9/20/19 6:25 AM, Erik Skultety wrote:
On Thu, Sep 19, 2019 at 05:04:47PM -0300, Daniel Henrique Barboza wrote:
A common operation in qemu_domain_address is comparing a virPCIDeviceAddress and assigning domain, bus, slot and function to a specific value. The former can be done with the existing virPCIDeviceAddressEqual() helper.
A new virpci helper called virPCIDeviceSetAddress() was created to make it easier to assign the same domain/bus/slot/function of an already existent virPCIDeviceAddress. Using both in qemu_domain_address made the code tidier, since the object was already created to use virPCIDeviceAddressEqual().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> ---
I wasn't sure if this change was worth contributing it back, since it feels more like a 'look and feel' change. But since it made the code inside qemu_domain_address tidier, I decided to take a shot. I actually think it does enhance readability, so why not. However...
[...]
--- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1729,45 +1729,41 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, /* Verify that first IDE and USB controllers (if any) is on the PIIX3, fn 1 */ for (i = 0; i < def->ncontrollers; i++) { virDomainControllerDefPtr cont = def->controllers[i]; + virPCIDeviceAddress primaryIDEAddr = {.domain = 0, .bus = 0, + .slot = 1, .function = 1}; + virPCIDeviceAddress piix3USBAddr = {.domain = 0, .bus = 0, + .slot = 1, .function = 2};
/* First IDE controller lives on the PIIX3 at slot=1, function=1 */ if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE && cont->idx == 0) { if (virDeviceInfoPCIAddressIsPresent(&cont->info)) { - if (cont->info.addr.pci.domain != 0 || - cont->info.addr.pci.bus != 0 || - cont->info.addr.pci.slot != 1 || - cont->info.addr.pci.function != 1) { + if (!virPCIDeviceAddressEqual(&cont->info.addr.pci, + &primaryIDEAddr)) { I think this virPCIDeviceAddressEqual consolidation might be a separate patch (matter of taste, feel free to disagree, but to me the changes are not strictly related and can be easily split).
virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary IDE controller must have PCI address 0:0:1.1")); + _("Primary IDE controller must have PCI " + "address 0:0:1.1"));
Unrelated...but it can stay...
return -1; } } else { cont->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - cont->info.addr.pci.domain = 0; - cont->info.addr.pci.bus = 0; - cont->info.addr.pci.slot = 1; - cont->info.addr.pci.function = 1; + virPCIDeviceSetAddress(&cont->info.addr.pci, &primaryIDEAddr);
So, given the signature, it feels more like a Copy rather than Set. I imagine a setter to enumerate the scalar types and the destination where the values should end up in, in this regard, it's not a proper setter because the struct has a few more elements that technically can be set, but if you created such a setter I'd say we didn't gain anything in terms of readability at all, especially when you could do:
/* provided that I'm interpreting 6.7.8.19 [1] of the C99 standard correctly, * everything else we're not using should be 0, otherwise we can't do this. * Alternatively, you'd need to reset the other members as well */ cont->info.addr.pci = primaryIDEAddr;
That was my first idea before trying to come out with this "partial setter" so to speak - since we're setting only domain/bus/slot/function, I wanted to avoid setting the other values as well. But now that I thought bit more about the context I believe the direct assignment here is fine indeed. Here's a snippet of the code I'm changing: if (virDeviceInfoPCIAddressIsPresent(&cont->info)) { (....code to check if address is equal to X ...) } else { /* assign address to X */ AddressIsPresent checks for info->type=TYPE_PCI and if addr.pci{domain|bus|slot} is not zero*. It didn't check if multi,ext and zpci was filled, but the current code doesn't care much either (otherwise the existing code would be setting those as well). My conclusion is that, at this point, these other attributes are either irrelevant or will be set up to later on, thus the default 0 values are fine here. I'll re-send this dude with direct assignment and without this extra setter function I came up with inside vircpi.c. Thanks for the review! DHB
I'm not completely sold on the helper the way it is now, but I'm up for the idea of the patch. Creating a proper Copy helper would bring the obvious issue of needing to retain the original settings of .multi and zPCI to make it universal, that's why I'm opting for the direct assignment, because all of the relevant code in the patch should be x86 only and multifunction would be reset to 0, which is also the default setting anyway, so that should play nicely as well.
[1] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf
Erik
participants (2)
-
Daniel Henrique Barboza -
Erik Skultety