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

[Yi Min Zhao]

在 2018/10/11 下午6:31, Andrea Bolognani 写道:
> 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.
OK.
>
> [...]
>> +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()
Sure.
> [...]
>>   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.
ok.
>
> [...]
>> @@ -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.
Yeah.
> [...]
>> @@ -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) {
>        ...
OK
>
> With all of the above addressed,
>
>    Reviewed-by: Andrea Bolognani <abologna at redhat.com>
>
Thanks!

-- 
Yi Min