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

[Yi Min Zhao]

在 2018/8/17 上午12:06, Andrea Bolognani 写道:
> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
> [...]
>> +static inline bool
>> +virDeviceInfoPCIAddressExtensionPresent(const virDomainDeviceInfo *info)
>> +{
>> +    return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
>> +        info->addr.pci.zpci;
>> +}
> This should be called virDeviceInfoPCIAddressExtensionIsPresent()
> since it's a predicate. I know the corresponding PCI function gets
> the naming wrong, but that doesn't mean you should too :)
>
> In the same vein, I don't think this (or the PCI version, for that
> matter) need to be inline... I'd rather have them both as regular
> functions in the .c file. I'll probably cook up a patch cleaning
> up the PCI part later, see what the feedback is.

Got it. Thank!
>
> [...]
>> +static int
>> +virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
>> +                                    virZPCIDeviceAddressPtr addr)
>> +{
>> +    if (!addr->uid_assigned &&
>> +        virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr)) {
>> +            return -1;
>> +    }
>> +
>> +    if (!addr->fid_assigned &&
>> +        virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr)) {
>> +        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
>> +        return -1;
>> +    }
> Not sure I get the logic here... ReserveNextAddress() is supposed
> to pick the next available address and reserve it, but IIUC this
> will skip picking either id based on whether they were assigned.
If uid/fid is assigned, we call ***Reserve***().
If uid/fid is unassigned, we call ***ReserveNext***().

But I'm not very clear about your concern.
> I don't think that's what we want.
>
> Also all functions that return an int should be called like
>
>    if (virDomainZPCIAddressReserveNextUid() < 0) {
>        ...
>    }

OK.

>
> [...]
>> +static int
>> +virDomainPCIAddressExtensionEnsureAddr(virDomainPCIAddressSetPtr addrs,
>> +                                       virDomainDeviceInfoPtr dev)
>> +{
> It's weird that this function doesn't get extFlags as an argument,
> unlike the other ones you've introduced. Better make it consistent.
We have to pass DeviceInfo which already has extFlags. If we pass 
extFlags again,
isn't it redundant? This function is only called in one place for 
hotplug case.
>
>> +    virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
>> +
>> +    if (zpci && !dev->pciAddressExtFlags) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported."));
>> +        return -1;
>> +    }
> The error message is very generic here, it should at the very least
> make it clear that zPCI is not supported *for the specific device*.
>
> I'm not sure this is the best place to perform that kind of check,
> either.
>
This is only called by virDomainPCIAddressEnsureAddr() for hotplug case.
I thought this again. If we remove those two boolean members from zPCI
address structure as what we discuss on the 1st patch, we could move this
check to virDomainPCIAddressEnsureAddr() like what PCI addr check does.