[PATCH RESEND 03/20] domain_driver.c: use PCI address with virDomainDriverNodeDeviceGetPCIInfo()
Laine Stump
laine at redhat.com
Thu Jan 21 01:05:26 UTC 2021
On 1/18/21 2:53 PM, Daniel Henrique Barboza wrote:
> Instead of receiving 4 uints in order and write domain/bus/slot/function,
> receive a virPCIDeviceAddressPtr instead and write into it.
>
> This change will allow us to simplify the API for virPCIDeviceNew()
> in the next patch.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
> ---
> src/hypervisor/domain_driver.c | 13 +++++--------
> src/hypervisor/domain_driver.h | 5 +----
> src/libxl/libxl_driver.c | 18 +++++++++---------
> src/qemu/qemu_driver.c | 18 +++++++++---------
> 4 files changed, 24 insertions(+), 30 deletions(-)
>
> diff --git a/src/hypervisor/domain_driver.c b/src/hypervisor/domain_driver.c
> index 68dbf10ac6..cdb53bcf3e 100644
> --- a/src/hypervisor/domain_driver.c
> +++ b/src/hypervisor/domain_driver.c
> @@ -341,20 +341,17 @@ virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
>
> int
> virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
> - unsigned *domain,
> - unsigned *bus,
> - unsigned *slot,
> - unsigned *function)
> + virPCIDeviceAddressPtr devAddr)
> {
> virNodeDevCapsDefPtr cap;
>
> cap = def->caps;
> while (cap) {
> if (cap->data.type == VIR_NODE_DEV_CAP_PCI_DEV) {
> - *domain = cap->data.pci_dev.domain;
> - *bus = cap->data.pci_dev.bus;
> - *slot = cap->data.pci_dev.slot;
> - *function = cap->data.pci_dev.function;
> + devAddr->domain = cap->data.pci_dev.domain;
> + devAddr->bus = cap->data.pci_dev.bus;
> + devAddr->slot = cap->data.pci_dev.slot;
> + devAddr->function = cap->data.pci_dev.function;
Looking at this, my first thought was "surely cap->data.pci_dev should
have a single virPCIDeviceAddress rather than the individual members!
Then those 4 lines could just be "devAddr = cap->data.pci_dev.address".
Except that somewhere along the way someone decided that the
virZPCIDeviceAddress should be a subelement of a virPCIDeviceAddress, so
there is extra stuff in there that's unwanted in this case (can't place
the blame entirely there of course :-) - even before that, there was the
extra "multi" setting, which is really an attribute of a PCI slot, not
an attribute of a PCI address.)
At any rate, what you've done is much better than what was there before,
and simplifications/consolidations like this will often lead to
identification of other simplifications/consolidations that can be made
later.
(Oh, and we mustn't forget that passing a single virPCIDeviceAddressPtr
instead of 4 int* makes it much less likely that someone will typo the
items in the wrong order)
Reviewed-by: Laine Stump <laine at redhat.com>
I'm really liking these so far! :-)
> break;
> }
>
> diff --git a/src/hypervisor/domain_driver.h b/src/hypervisor/domain_driver.h
> index 2bb053d559..86b92d0284 100644
> --- a/src/hypervisor/domain_driver.h
> +++ b/src/hypervisor/domain_driver.h
> @@ -48,7 +48,4 @@ int virDomainDriverSetupPersistentDefBlkioParams(virDomainDefPtr persistentDef,
> int nparams);
>
> int virDomainDriverNodeDeviceGetPCIInfo(virNodeDeviceDefPtr def,
> - unsigned *domain,
> - unsigned *bus,
> - unsigned *slot,
> - unsigned *function);
> + virPCIDeviceAddressPtr devAddr);
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 0821d39c9b..360d553a22 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5780,7 +5780,7 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
> unsigned int flags)
> {
> virPCIDevicePtr pci = NULL;
> - unsigned domain = 0, bus = 0, slot = 0, function = 0;
> + virPCIDeviceAddress devAddr;
> int ret = -1;
> virNodeDeviceDefPtr def = NULL;
> char *xml = NULL;
> @@ -5815,10 +5815,10 @@ libxlNodeDeviceDetachFlags(virNodeDevicePtr dev,
> if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
> goto cleanup;
>
> - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
> + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
> goto cleanup;
>
> - pci = virPCIDeviceNew(domain, bus, slot, function);
> + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function);
> if (!pci)
> goto cleanup;
>
> @@ -5853,7 +5853,7 @@ static int
> libxlNodeDeviceReAttach(virNodeDevicePtr dev)
> {
> virPCIDevicePtr pci = NULL;
> - unsigned domain = 0, bus = 0, slot = 0, function = 0;
> + virPCIDeviceAddress devAddr;
> int ret = -1;
> virNodeDeviceDefPtr def = NULL;
> char *xml = NULL;
> @@ -5886,10 +5886,10 @@ libxlNodeDeviceReAttach(virNodeDevicePtr dev)
> if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
> goto cleanup;
>
> - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
> + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
> goto cleanup;
>
> - pci = virPCIDeviceNew(domain, bus, slot, function);
> + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function);
> if (!pci)
> goto cleanup;
>
> @@ -5911,7 +5911,7 @@ static int
> libxlNodeDeviceReset(virNodeDevicePtr dev)
> {
> virPCIDevicePtr pci = NULL;
> - unsigned domain = 0, bus = 0, slot = 0, function = 0;
> + virPCIDeviceAddress devAddr;
> int ret = -1;
> virNodeDeviceDefPtr def = NULL;
> char *xml = NULL;
> @@ -5944,10 +5944,10 @@ libxlNodeDeviceReset(virNodeDevicePtr dev)
> if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0)
> goto cleanup;
>
> - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
> + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
> goto cleanup;
>
> - pci = virPCIDeviceNew(domain, bus, slot, function);
> + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function);
> if (!pci)
> goto cleanup;
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0a732a241d..28781cc34b 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -11962,7 +11962,7 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
> {
> virQEMUDriverPtr driver = dev->conn->privateData;
> virPCIDevicePtr pci = NULL;
> - unsigned domain = 0, bus = 0, slot = 0, function = 0;
> + virPCIDeviceAddress devAddr;
> int ret = -1;
> virNodeDeviceDefPtr def = NULL;
> g_autofree char *xml = NULL;
> @@ -11997,10 +11997,10 @@ qemuNodeDeviceDetachFlags(virNodeDevicePtr dev,
> if (virNodeDeviceDetachFlagsEnsureACL(dev->conn, def) < 0)
> goto cleanup;
>
> - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
> + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
> goto cleanup;
>
> - pci = virPCIDeviceNew(domain, bus, slot, function);
> + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function);
> if (!pci)
> goto cleanup;
>
> @@ -12046,7 +12046,7 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev)
> {
> virQEMUDriverPtr driver = dev->conn->privateData;
> virPCIDevicePtr pci = NULL;
> - unsigned domain = 0, bus = 0, slot = 0, function = 0;
> + virPCIDeviceAddress devAddr;
> int ret = -1;
> virNodeDeviceDefPtr def = NULL;
> g_autofree char *xml = NULL;
> @@ -12078,10 +12078,10 @@ qemuNodeDeviceReAttach(virNodeDevicePtr dev)
> if (virNodeDeviceReAttachEnsureACL(dev->conn, def) < 0)
> goto cleanup;
>
> - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
> + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
> goto cleanup;
>
> - pci = virPCIDeviceNew(domain, bus, slot, function);
> + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function);
> if (!pci)
> goto cleanup;
>
> @@ -12100,7 +12100,7 @@ qemuNodeDeviceReset(virNodeDevicePtr dev)
> {
> virQEMUDriverPtr driver = dev->conn->privateData;
> virPCIDevicePtr pci;
> - unsigned domain = 0, bus = 0, slot = 0, function = 0;
> + virPCIDeviceAddress devAddr;
> int ret = -1;
> virNodeDeviceDefPtr def = NULL;
> g_autofree char *xml = NULL;
> @@ -12132,10 +12132,10 @@ qemuNodeDeviceReset(virNodeDevicePtr dev)
> if (virNodeDeviceResetEnsureACL(dev->conn, def) < 0)
> goto cleanup;
>
> - if (virDomainDriverNodeDeviceGetPCIInfo(def, &domain, &bus, &slot, &function) < 0)
> + if (virDomainDriverNodeDeviceGetPCIInfo(def, &devAddr) < 0)
> goto cleanup;
>
> - pci = virPCIDeviceNew(domain, bus, slot, function);
> + pci = virPCIDeviceNew(devAddr.domain, devAddr.bus, devAddr.slot, devAddr.function);
> if (!pci)
> goto cleanup;
>
More information about the libvir-list
mailing list