[libvirt] [PATCH 2/2] virpci, qemu_domain_address: adding virPCIDeviceSetAddress

[Erik Skultety]

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