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

[Yi Min Zhao]

在 2018/9/11 下午9:59, 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.
Good catch!
>
> [...]
>> +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.
>
> [...]
OK.
>> +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.
OK.
>
> [...]
>> +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.
Actually there's the function ***ReleaseId() like you said. Don't you 
see it?
>
> 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.
Sure.
>
> [...]
>> +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?
I think it's hard to do it as what you said. We process assigned zpci 
addresses
firstly. And then reserve next address for empty zpci addresses. If we don't
check this, we might reserve an address for a reserved one.
>
> [...]
>> +void
>> +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
>> +                                        virPCIDeviceAddressPtr addr,
>> +                                        int extFlags)
>> +{
>> +    if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))
> You don't need two sets of parentheses here :)
>
yes. typo.

-- 
Yi Min