[libvirt] [PATCH v3 05/18] conf: new function virDomainPCIAddressReserveNextAddr()

[Andrea Bolognani]

On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
> There is an existing virDomainPCIAddressReserveNextSlot() which will
> reserve all functions of the next available PCI slot. One place in the
> qemu PCI address assignment code requires reserving a *single*
> function of the next available PCI slot. This patch modifies and
> renames virDomainPCIAddressReserveNextSlot() so that it can fulfill
> both the original purpose and the need to reserve a single function.
> 
> (This is being done so that the abovementioned code in qemu can have
> its "kind of open coded" solution replaced with a call to this new
> function).
> ---
>  src/conf/domain_addr.c   | 50 +++++++++++++++++++++++++++++++++++++++++++-----
>  src/conf/domain_addr.h   |  7 +++++++
>  src/libvirt_private.syms |  1 +
>  3 files changed, 53 insertions(+), 5 deletions(-)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index cff2c3b..f735fb4 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -691,29 +691,69 @@ virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
>      return 0;
>  }
>  
> +/**
> + * virDomainPCIAddressReserveNextAddr:
> + *
> + *    find the next *completely unreserved* slot with compatible
> + *    connection @flags, and mark either one function or the entire
> + *    slot (according to @function and @reserveEntireSlot).

Indentation is weird, here and below.

We don't seem to be parsing and formatting the documentation
for internal functions such as this one, still...

> + * @addrs:     a set of addresses - one bit for each function on each
> + *             slot on each bus, set if in use, clear if not in use.

I don't think you need to explain virDomainPCIAddressSetPtr
here, especially not down to how it's implemented :)

> + * @dev:       virDomainDeviceInfo that should get the new address set.
> + *
> + * @flags:     CONNECT_TYPE flags for the port we're looking for
> + *
> + * @function:  which function on the slot to mark as reserved
> + *             (if @reserveEntireSlot is false)
> + *
> + * @reserveEntireSlot: true to reserve all functions on the new slot,
> + *              false to reserve just @function
> + *
> + *
> + * returns 0 (and dev->addr.pci set) on success, or -1 on failure.

Side effects such as setting the PCI address for @dev should
be part of the description, not here.

> + */
>  int
> -virDomainPCIAddressReserveNextSlot(virDomainPCIAddressSetPtr addrs,
> +virDomainPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
>                                     virDomainDeviceInfoPtr dev,
> -                                   virDomainPCIConnectFlags flags)
> +                                   virDomainPCIConnectFlags flags,
> +                                   unsigned int function,
> +                                   bool reserveEntireSlot)
>  {
>      virPCIDeviceAddress addr;
> +
>      if (virDomainPCIAddressGetNextSlot(addrs, &addr, flags) < 0)
>          return -1;
>  
> -    if (virDomainPCIAddressReserveSlot(addrs, &addr, flags) < 0)
> +    addr.function = reserveEntireSlot ? 0 : function;
> +
> +    if (virDomainPCIAddressReserveAddr(addrs, &addr, flags, reserveEntireSlot, false) < 0)
>          return -1;
>  
> +    addrs->lastaddr = addr;
> +    addrs->lastaddr.function = 0;
> +    addrs->lastaddr.multi = VIR_TRISTATE_SWITCH_ABSENT;

Do we need to explicitly set function = 0 and multi = ABSENT?
AFAICT only domain, bus and slot are used in the context of
virDomainPCIAddressSet. And if that wasn't the case, they
should be set according to the last address that was
reserved, including function.

Everything else looks good :)

-- 
Andrea Bolognani / Red Hat / Virtualization