Re: [PATCH v6 06/13] conf: Introduce address caching for PCI extensions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2018-09-28 at 16:46 +0800, Yi Min Zhao wrote:
[...]
> +static void
> +virDomainPCIAddressSetExtensionFree(virDomainPCIAddressSetPtr addrs)
> +{
> +    if (!addrs || !addrs->zpciIds)
> +        return;
> +
> +    virHashFree(addrs->zpciIds->uids);
> +    virHashFree(addrs->zpciIds->fids);
> +    VIR_FREE(addrs->zpciIds);
> +}

Please keep the Free() function closer to the Alloc() function.

[...]
> +int
> +virDomainPCIAddressSetExtensionAlloc(virDomainPCIAddressSetPtr addrs,
> +                                     virPCIDeviceAddressExtensionFlags extFlags)
> +{
> +    if (extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +        if (addrs->zpciIds)
> +            return 0;
> +
> +        if (VIR_ALLOC(addrs->zpciIds) < 0)
> +            return -1;
> +
> +        if (!(addrs->zpciIds->uids = virHashCreateFull(10, NULL,
> +                                                       virZPCIAddrCode,
> +                                                       virZPCIAddrEqual,
> +                                                       virZPCIAddrCopy,
> +                                                       virZPCIAddrKeyFree)))

The function names are inconsistent here: they all deal with hash
keys, but only the free function contains the word "key" in its
name.

Please change them like so:

  virZPCIAddrCode()  => virZPCIAddrKeyCode()
  virZPCIAddrEqual() => virZPCIAddrKeyEqual()
  virZPCIAddrCopy()  => virZPCIAddrKeyCopy()

[...]
>  typedef struct _virDomainPCIAddressSet virDomainPCIAddressSet;
>  typedef virDomainPCIAddressSet *virDomainPCIAddressSetPtr;
>  
> +char *virDomainPCIAddressAsString(virPCIDeviceAddressPtr addr)
> +      ATTRIBUTE_NONNULL(1);

This hunk is a leftover from a previous respin, please drop it.

[...]
> @@ -124,6 +124,7 @@ virDomainPCIAddressReserveAddr;
>  virDomainPCIAddressReserveNextAddr;
>  virDomainPCIAddressSetAllMulti;
>  virDomainPCIAddressSetAlloc;
> +virDomainPCIAddressSetExtensionAlloc;

Hm. Instead of exporting this function just so you can call it from
qemuDomainPCIAddressSetCreate(), wouldn't it make more sense to call
it directly from inside virDomainPCIAddressSetAlloc()?

You'd have to pass virPCIDeviceAddressExtensionFlags to the latter,
of course, but that sounds fairly sensible; the only other user,
the bhyve driver, can just pass _NONE.

[...]
> @@ -1509,6 +1509,13 @@ qemuDomainPCIAddressSetCreate(virDomainDefPtr def,
>  
>      addrs->dryRun = dryRun;
>  
> +    /* create zpci address set for s390 domain */
> +    if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ZPCI) &&
> +        virDomainPCIAddressSetExtensionAlloc(addrs,
> +                                             VIR_PCI_ADDRESS_EXTENSION_ZPCI)) {

This should be

  if (... &&
      virDomainPCIAddressSetExtensionAlloc(...) < 0) {
      ...


With all of the above addressed,

  Reviewed-by: Andrea Bolognani <abologna@xxxxxxxxxx>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux