3:04 p.m.
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
3:04 p.m.
New subject: [libvirt] [PATCH 1/2] qemu_domain_address.c: use VIR_AUTOFREE() in strings
A few 'cleanup' labels gone after using VIR_AUTOFREE() in the
strings.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
3 a.m.
New subject: [libvirt] [PATCH 1/2] qemu_domain_address.c: use VIR_AUTOFREE() in strings
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(a)redhat.com>
3:04 p.m.
New subject: [libvirt] [PATCH 2/2] virpci, qemu_domain_address: adding virPCIDeviceSetAddress
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(a)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
4:25 a.m.
New subject: [libvirt] [PATCH 2/2] virpci, qemu_domain_address: adding virPCIDeviceSetAddress
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(a)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
7:39 a.m.
New subject: [libvirt] [PATCH 2/2] virpci, qemu_domain_address: adding virPCIDeviceSetAddress
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(a)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
1890
days inactive
1891
days old
5 comments
2 participants
participants (2)
-
Daniel Henrique Barboza -
Erik Skultety