[libvirt] [PATCH v2 0/3] conf: Move stuff out of domain_addr
$ mv blurb here/ Andrea Bolognani (3): conf: Move virDomainPCIAddressAsString() to util/virpci conf: Rename virDomainPCIAddressAsString() util: Drop virPCIGetAddrString() src/conf/domain_addr.c | 20 +++----------------- src/conf/domain_addr.h | 4 ---- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain_address.c | 6 +++--- src/util/virnetdev.c | 6 +----- src/util/virpci.c | 21 +++++++++------------ src/util/virpci.h | 8 ++------ 8 files changed, 20 insertions(+), 49 deletions(-) -- 2.17.1
It's a better fit than conf/domain_conf. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 14 -------------- src/conf/domain_addr.h | 4 ---- src/libvirt_private.syms | 2 +- src/util/virpci.c | 13 +++++++++++++ src/util/virpci.h | 3 +++ 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index b62fd53c66..d2e1142462 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -573,20 +573,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, } -char * -virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) -{ - char *str; - - ignore_value(virAsprintf(&str, "%.4x:%.2x:%.2x.%.1x", - addr->domain, - addr->bus, - addr->slot, - addr->function)); - return str; -} - - /* * Check if the PCI slot is used by another device. */ diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5ad9d8ef3d..f8ab742715 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -124,9 +124,6 @@ struct _virDomainPCIAddressSet { typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr; -char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) - ATTRIBUTE_NONNULL(1); - virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses); void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); @@ -138,7 +135,6 @@ bool virDomainPCIAddressValidate(virDomainPCIAddressSetPtr addrs, bool fromConfig) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); - int virDomainPCIAddressBusSetModel(virDomainPCIAddressBusPtr bus, virDomainControllerModelPCI model) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e2340d23a6..04242a22b1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -119,7 +119,6 @@ virPCIDeviceAddressParseXML; virDomainCCWAddressAssign; virDomainCCWAddressSetCreateFromDomain; virDomainCCWAddressSetFree; -virDomainPCIAddressAsString; virDomainPCIAddressBusIsFullyReserved; virDomainPCIAddressBusSetModel; virDomainPCIAddressEnsureAddr; @@ -2497,6 +2496,7 @@ virObjectUnref; # util/virpci.h +virDomainPCIAddressAsString; virPCIDeviceAddressGetIOMMUGroupAddresses; virPCIDeviceAddressGetIOMMUGroupNum; virPCIDeviceAddressGetSysfsFile; diff --git a/src/util/virpci.c b/src/util/virpci.c index 6cf2acf2d1..512e365cad 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1684,6 +1684,19 @@ virPCIGetAddrString(unsigned int domain, return 0; } +char * +virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) +{ + char *str; + + ignore_value(virAsprintf(&str, "%.4x:%.2x:%.2x.%.1x", + addr->domain, + addr->bus, + addr->slot, + addr->function)); + return str; +} + virPCIDevicePtr virPCIDeviceNew(unsigned int domain, unsigned int bus, diff --git a/src/util/virpci.h b/src/util/virpci.h index 2ac87694df..9ef1b838b7 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -225,6 +225,9 @@ int virPCIGetAddrString(unsigned int domain, char **pciConfigAddr) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; +char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) + ATTRIBUTE_NONNULL(1); + int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, -- 2.17.1
On Wed, Sep 05, 2018 at 10:09:24AM +0200, Andrea Bolognani wrote:
It's a better fit than conf/domain_conf.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 14 -------------- src/conf/domain_addr.h | 4 ---- src/libvirt_private.syms | 2 +- src/util/virpci.c | 13 +++++++++++++ src/util/virpci.h | 3 +++ 5 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index b62fd53c66..d2e1142462 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -573,20 +573,6 @@ virDomainPCIAddressSetGrow(virDomainPCIAddressSetPtr addrs, }
-char * -virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) -{ - char *str; - - ignore_value(virAsprintf(&str, "%.4x:%.2x:%.2x.%.1x", - addr->domain, - addr->bus, - addr->slot, - addr->function)); - return str; -} - - /* * Check if the PCI slot is used by another device. */ diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h index 5ad9d8ef3d..f8ab742715 100644 --- a/src/conf/domain_addr.h +++ b/src/conf/domain_addr.h @@ -124,9 +124,6 @@ struct _virDomainPCIAddressSet { typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet; typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
-char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) - ATTRIBUTE_NONNULL(1); - virDomainPCIAddressSetPtr virDomainPCIAddressSetAlloc(unsigned int nbuses);
void virDomainPCIAddressSetFree(virDomainPCIAddressSetPtr addrs); @@ -138,7 +135,6 @@ bool virDomainPCIAddressValidate(virDomainPCIAddressSetPtr addrs, bool fromConfig) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
-
Separate this into trivial patch. Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
The struct is called virPCIDeviceAddress and the functions operating on it should be named accordingly. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 6 +++--- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain_address.c | 6 +++--- src/util/virpci.c | 2 +- src/util/virpci.h | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c index d2e1142462..442e6aab94 100644 --- a/src/conf/domain_addr.c +++ b/src/conf/domain_addr.c @@ -604,7 +604,7 @@ virDomainPCIAddressReserveAddrInternal(virDomainPCIAddressSetPtr addrs, virErrorNumber errType = (fromConfig ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR); - if (!(addrStr = virDomainPCIAddressAsString(addr))) + if (!(addrStr = virPCIDeviceAddressAsString(addr))) goto cleanup; /* Add an extra bus if necessary */ @@ -689,7 +689,7 @@ virDomainPCIAddressEnsureAddr(virDomainPCIAddressSetPtr addrs, if (!flags) return 0; - if (!(addrStr = virDomainPCIAddressAsString(&dev->addr.pci))) + if (!(addrStr = virPCIDeviceAddressAsString(&dev->addr.pci))) goto cleanup; if (virDeviceInfoPCIAddressIsPresent(dev)) { @@ -770,7 +770,7 @@ virDomainPCIAddressFindUnusedFunctionOnBus(virDomainPCIAddressBusPtr bus, *found = false; - if (!(addrStr = virDomainPCIAddressAsString(searchAddr))) + if (!(addrStr = virPCIDeviceAddressAsString(searchAddr))) goto cleanup; if (!virDomainPCIAddressFlagsCompatible(searchAddr, addrStr, bus->flags, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 04242a22b1..0fc5314b02 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2496,7 +2496,7 @@ virObjectUnref; # util/virpci.h -virDomainPCIAddressAsString; +virPCIDeviceAddressAsString; virPCIDeviceAddressGetIOMMUGroupAddresses; virPCIDeviceAddressGetIOMMUGroupNum; virPCIDeviceAddressGetSysfsFile; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8aa20496bc..b72506f4e1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -301,7 +301,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { size_t i; - if (!(devStr = virDomainPCIAddressAsString(&info->addr.pci))) + if (!(devStr = virPCIDeviceAddressAsString(&info->addr.pci))) goto cleanup; for (i = 0; i < domainDef->ncontrollers; i++) { virDomainControllerDefPtr cont = domainDef->controllers[i]; diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index dda14adeb3..24fdf12128 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1306,7 +1306,7 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, * inappropriate address types. */ if (!info->pciConnectFlags) { - char *addrStr = virDomainPCIAddressAsString(&info->addr.pci); + char *addrStr = virPCIDeviceAddressAsString(&info->addr.pci); VIR_WARN("qemuDomainDeviceCalculatePCIConnectFlags() thinks that the " "device with PCI address %s should not have a PCI address", @@ -1554,7 +1554,7 @@ qemuDomainValidateDevicePCISlotsPIIX3(virDomainDefPtr def, memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 2; - if (!(addrStr = virDomainPCIAddressAsString(&tmp_addr))) + if (!(addrStr = virPCIDeviceAddressAsString(&tmp_addr))) goto cleanup; if (!virDomainPCIAddressValidate(addrs, &tmp_addr, addrStr, flags, true)) @@ -1743,7 +1743,7 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, memset(&tmp_addr, 0, sizeof(tmp_addr)); tmp_addr.slot = 1; - if (!(addrStr = virDomainPCIAddressAsString(&tmp_addr))) + if (!(addrStr = virPCIDeviceAddressAsString(&tmp_addr))) goto cleanup; if (!virDomainPCIAddressValidate(addrs, &tmp_addr, addrStr, flags, true)) diff --git a/src/util/virpci.c b/src/util/virpci.c index 512e365cad..d82da710ee 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1685,7 +1685,7 @@ virPCIGetAddrString(unsigned int domain, } char * -virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) +virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) { char *str; diff --git a/src/util/virpci.h b/src/util/virpci.h index 9ef1b838b7..271a753be2 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -225,7 +225,7 @@ int virPCIGetAddrString(unsigned int domain, char **pciConfigAddr) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; -char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr) +char *virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1); int virPCIDeviceAddressParse(char *address, virPCIDeviceAddressPtr bdf); -- 2.17.1
On Wed, Sep 05, 2018 at 10:09:25AM +0200, Andrea Bolognani wrote:
The struct is called virPCIDeviceAddress and the functions operating on it should be named accordingly.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/conf/domain_addr.c | 6 +++--- src/libvirt_private.syms | 2 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_domain_address.c | 6 +++--- src/util/virpci.c | 2 +- src/util/virpci.h | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-)
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
There's a single user for it which takes an existing virPCIDeviceAddress, passes its various bits to the function which in turn constructs a virPCIDevice and then copies the string representation for the caller to use: we can use virPCIDeviceAddressAsString() instead and avoid creating the virPCIDevice in the first place. Since the function ends up having no users after the change, we can just drop it. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virnetdev.c | 6 +----- src/util/virpci.c | 16 ---------------- src/util/virpci.h | 7 ------- 3 files changed, 1 insertion(+), 28 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8eac419725..9cc9d18155 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1305,11 +1305,7 @@ virNetDevGetVirtualFunctions(const char *pfname, goto cleanup; for (i = 0; i < *n_vfname; i++) { - if (virPCIGetAddrString((*virt_fns)[i]->domain, - (*virt_fns)[i]->bus, - (*virt_fns)[i]->slot, - (*virt_fns)[i]->function, - &pciConfigAddr) < 0) { + if (!(pciConfigAddr = virPCIDeviceAddressAsString((*virt_fns)[i]))) { virReportSystemError(ENOSYS, "%s", _("Failed to get PCI Config Address String")); goto cleanup; diff --git a/src/util/virpci.c b/src/util/virpci.c index d82da710ee..1730d888f7 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1668,22 +1668,6 @@ virPCIDeviceReadID(virPCIDevicePtr dev, const char *id_name) return id_str; } -int -virPCIGetAddrString(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function, - char **pciConfigAddr) -{ - VIR_AUTOPTR(virPCIDevice) dev = NULL; - - dev = virPCIDeviceNew(domain, bus, slot, function); - if (!dev || VIR_STRDUP(*pciConfigAddr, dev->name) < 0) - return -1; - - return 0; -} - char * virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) { diff --git a/src/util/virpci.h b/src/util/virpci.h index 271a753be2..b4f72f8f06 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -218,13 +218,6 @@ int virPCIGetSysfsFile(char *virPCIDeviceName, char **pci_sysfs_device_link) ATTRIBUTE_RETURN_CHECK; -int virPCIGetAddrString(unsigned int domain, - unsigned int bus, - unsigned int slot, - unsigned int function, - char **pciConfigAddr) - ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; - char *virPCIDeviceAddressAsString(virPCIDeviceAddressPtr addr) ATTRIBUTE_NONNULL(1); -- 2.17.1
On Wed, Sep 05, 2018 at 10:09:26AM +0200, Andrea Bolognani wrote:
There's a single user for it which takes an existing virPCIDeviceAddress, passes its various bits to the function which in turn constructs a virPCIDevice and then copies the string representation for the caller to use: we can use virPCIDeviceAddressAsString() instead and avoid creating the virPCIDevice in the first place. Since the function ends up having no users after the change, we can just drop it.
Much wrapping, such narrow =)
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virnetdev.c | 6 +----- src/util/virpci.c | 16 ---------------- src/util/virpci.h | 7 ------- 3 files changed, 1 insertion(+), 28 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 8eac419725..9cc9d18155 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -1305,11 +1305,7 @@ virNetDevGetVirtualFunctions(const char *pfname, goto cleanup;
for (i = 0; i < *n_vfname; i++) { - if (virPCIGetAddrString((*virt_fns)[i]->domain, - (*virt_fns)[i]->bus, - (*virt_fns)[i]->slot, - (*virt_fns)[i]->function, - &pciConfigAddr) < 0) { + if (!(pciConfigAddr = virPCIDeviceAddressAsString((*virt_fns)[i]))) {
There is one more thing that this change does, previously it would fail for non-existing device, which it will not now. I don't think that is a problem, but wanted to mention that.
virReportSystemError(ENOSYS, "%s", _("Failed to get PCI Config Address String"));
You should remove this error message as virAsprintf() in virPCIDeviceAddressAsString() already sets an OOM Error. I you like working on this function there are lot of things that leak from the loop, so looking at that would be great, thanks. With the removal of the error message above: Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
On Wed, 2018-09-05 at 14:53 +0200, Martin Kletzander wrote:
On Wed, Sep 05, 2018 at 10:09:26AM +0200, Andrea Bolognani wrote:
for (i = 0; i < *n_vfname; i++) { - if (virPCIGetAddrString((*virt_fns)[i]->domain, - (*virt_fns)[i]->bus, - (*virt_fns)[i]->slot, - (*virt_fns)[i]->function, - &pciConfigAddr) < 0) { + if (!(pciConfigAddr = virPCIDeviceAddressAsString((*virt_fns)[i]))) {
There is one more thing that this change does, previously it would fail for non-existing device, which it will not now. I don't think that is a problem, but wanted to mention that.
Well spotted; in this case, however, the addresses were obtained directly from sysfs through virPCIGetVirtualFunctions() so, as you mention, validating them again is unnecessary.
virReportSystemError(ENOSYS, "%s", _("Failed to get PCI Config Address String"));
You should remove this error message as virAsprintf() in virPCIDeviceAddressAsString() already sets an OOM Error.
Done.
I you like working on this function there are lot of things that leak from the loop, so looking at that would be great, thanks.
I'll see what I can do :) -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Martin Kletzander