[libvirt] [PATCH v5 09/13] conf: Allocate/release 'uid' and 'fid' in PCI address

[Andrea Bolognani]

On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> If the user
> define zPCI extension address but zPCI capability doesn't exist, an
> error will be reported.

You're (no longer) checking for the capability here, so the commit
message should be updated accordingly.

[...]
> +bool
> +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
> +{
> +    return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +        !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);

The second line is not aligned properly.

Actually, you'll probably want to restructure this a bit so that it
only looks into info->addr.pci.zpci if the zPCI extension flag is
present, after which the comment above will likely no longer apply.

[...]
> +static int
> +virDomainZPCIAddressReserveId(virHashTablePtr set,
> +                              unsigned int id,
> +                              const char *name)
> +{
> +    if (virHashLookup(set, &id)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("zPCI %s %u is already reserved"),
> +                       name, id);

Please format the id as octal for easier debugging when something
goes wrong. Same below.

[...]
> +static void
> +virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> +                               virZPCIDeviceAddressPtr addr)
> +{
> +    if (virHashRemoveEntry(set, &addr->uid) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Release uid %u failed"), addr->uid);
> +    }

You should have a generic virDomainZPCIAddressReleaseId() function
that you call from ReleaseUid() and ReleaseFid(), just like you
have for reserving them.

Additionally, it looks like failure to release an id is not a
fatal error since processing continue, so you should use VIR_WARN()
or something like that instead of virReportError() when that
happens.

[...]
> +int
> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +                                            virPCIDeviceAddressPtr dev,
> +                                            virDomainPCIAddressExtensionFlags extFlags)
> +{
> +    if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> +        virZPCIDeviceAddressIsEmpty(&dev->zpci)) {

You shouldn't need the second check: just go ahead and reserve the
next address regardless of what's currently stored in the device
info, no?

[...]
> +void
> +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
> +                                        virPCIDeviceAddressPtr addr,
> +                                        int extFlags)
> +{
> +    if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))

You don't need two sets of parentheses here :)

-- 
Andrea Bolognani / Red Hat / Virtualization