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

[Andrea Bolognani]

On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> This patch adds new functions for reservation, assignment and release
> to handle the uid/fid. If the uid/fid is defined in the domain XML,
> they will be reserved directly in collecting phase. If any of them is
> not defined, we will find out an available value for it from zPCI
> address hashtable, and reserve it. For hotplug case, there might be or
> not zPCI definition. So allocate and reserve uid/fid for undefined
> case. Assign if needed and reserve uid/fid for defined case. If the user
> define zPCI extension address but zPCI capability doesn't exist, an
> error will be reported.

For this patch I once again didn't look too closely to the
implementation, sorry.

[...]
> +static int
> +virDomainZPCIAddressReserveId(virHashTablePtr set, unsigned int id,
> +                              const char *name)

One argument per line, please.

There are more instances in the patch, but I'm not going to
point them all out :)

[...]
> +static int
> +virDomainZPCIAddressAssignUid(virHashTablePtr set, virZPCIDeviceAddressPtr addr)
> +{
> +    if (addr->uid_assigned)
> +        return 0;
> +
> +    addr->uid_assigned = virDomainZPCIAddressAssignId(set, &addr->zpci_uid, 1,
> +                                       VIR_DOMAIN_DEVICE_ZPCI_MAX_UID, "uid");

Messed up alignment. More instances further down.

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

Curly braces are required here. More instances further down.


Looking at

> +static void
> +virDomainZPCIAddressReleaseFid(virHashTablePtr set, virZPCIDeviceAddressPtr addr)

and

> +static void
> +virDomainZPCIAddressReleaseIds(virDomainZPCIAddressIdsPtr zpciIds,
> +                               virPCIDeviceAddressPtr addr)

and

> +static int
> +virDomainZPCIAddressReserveAddr(virDomainPCIAddressSetPtr addrs,
> +                                virPCIDeviceAddressPtr addr)

you're being awfully inconsistent about the datatypes you're passing
around...

Unless I've missed something that makes doing so impossible, please
try to make it so only the top-level datatypes (DomainPCIAddressSet
and PCIDeviceAddress) are passed around.

[...]
> +static int
> +virDomainZPCIAddressReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +                                    virZPCIDeviceAddressPtr zpci)
> +{
> +    if (!zpci->uid_assigned &&
> +        virDomainZPCIAddressReserveNextUid(addrs->zpciIds->uids, zpci))
> +            return -1;

Messed up indentation. Also, missing curly braces.

[...]
> +int
> +virDomainPCIAddressExtensionReserveAddr(virDomainPCIAddressSetPtr addrs,
> +                                        virPCIDeviceAddressPtr addr,
> +                                        virDomainPCIAddressExtensionFlags extFlags)
> +{
> +    if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +    /* Reserve uid/fid to ZPCI device which has defined uid/fid
> +     * in the domain.
> +     */

Messed up indentation.

[...]
> +int
> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> +                                            virPCIDeviceAddressPtr dev,
> +                                            virDomainPCIAddressExtensionFlags extFlags)
> +{
> +    if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +    /* Assign and reserve uid/fid to ZPCI device which has not defined uid/fid
> +     * in the domain.
> +     */

Messed up indentation.

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

This should be virDomainPCIAddressExtensionEnsureAddr() for
consistency's sake.

> @@ -1385,7 +1403,12 @@ qemuDomainCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED,
>           * parent, and will have its address collected during the scan
>           * of the parent's device type.
>          */
> -        return 0;
> +        if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ||
> +            info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
> +            return virDomainPCIAddressExtensionReserveAddr(addrs, addr,
> +                                                           info->pciAddressExtFlags);
> +        else
> +            return 0;

This doesn't look right: the comment specifically states that the
PCI address will be handled by the parent device in this case,
why wouldn't the zPCI address not be handled in the same way?

-- 
Andrea Bolognani / Red Hat / Virtualization