[libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +static inline bool
> +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
> +{
> +    return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +        info->addr.pci.zpci;
> +}

This should be called virDeviceInfoPCIAddressExtensionIsPresent()
since it's a predicate. I know the corresponding PCI function gets
the naming wrong, but that doesn't mean you should too :)

In the same vein, I don't think this (or the PCI version, for that
matter) need to be inline... I'd rather have them both as regular
functions in the .c file. I'll probably cook up a patch cleaning
up the PCI part later, see what the feedback is.

[...]
> +static int
> +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> +                                    virZPCIDeviceAddressPtr addr)
> +{
> +    if (!addr->uid_assigned &&
> +        virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
> +            return -1;
> +    }
> +
> +    if (!addr->fid_assigned &&
> +        virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
> +        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> +        return -1;
> +    }

Not sure I get the logic here... ReserveNextAddress() is supposed
to pick the next available address and reserve it, but IIUC this
will skip picking either id based on whether they were assigned.
I don't think that's what we want.

Also all functions that return an int should be called like

  if (virDomainZPCIAddressReserveNextUid() < 0) {
      ...
  }

[...]
> +static int
> +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
> +                                       virDomainDeviceInfoPtr dev)
> +{

It's weird that this function doesn't get extFlags as an argument,
unlike the other ones you've introduced. Better make it consistent.

> +    virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
> +
> +    if (zpci && !dev->pciAddressExtFlags) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported."));
> +        return -1;
> +    }

The error message is very generic here, it should at the very least
make it clear that zPCI is not supported *for the specific device*.

I'm not sure this is the best place to perform that kind of check,
either.

-- 
Andrea Bolognani / Red Hat / Virtualization