Re: [PATCH RESEND 03/20] domain_driver.c: use PCI address with virDomainDriverNodeDeviceGetPCIInfo()

[Laine Stump <laine@xxxxxxxxxx>]

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@xxxxxxxxx>
> ---
>   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@xxxxxxxxxx>


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;
>