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

Yi Min Zhao zyimin at linux.ibm.com
Thu Aug 23 10:01:57 UTC 2018



在 2018/8/22 下午5:56, Andrea Bolognani 写道:
> On Wed, 2018-08-22 at 11:00 +0800, Yi Min Zhao wrote:
>> 在 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!
> The patches I said I'd write are now on the list:
>
>    https://www.redhat.com/archives/libvir-list/2018-August/msg01105.html
>
> No reviews yet, we'll see whether they get ACKed or NACKed :)
I saw them. Could I give my ACKed?
>
>>> [...]
>>>> +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.
> That in ReserveNext() you're checking whether addr->*id_assigned
> when you know that ids have not been assigned or you wouldn't have
> called ReserveNext() in the first place... It seems unnecessary
> and confusing to me, so unless I've missed something that makes it
> necessary please just drop those checks.
As our discussion on the 1st patch, boolean values are removed.
So we don't need checks here.
>
>>> [...]
>>>> +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.
> I wanted the API to be more consistent but I realize now you have
> to pass either virPCIDeviceAddress or virDomainDeviceInfo depending
> on the context, so it doesn't really matter, you can leave it as it
> is. The signatures for the corresponding PCI functions are not
> entirely consistent either :)
>
I have a question about your previous comment about error report.
You thought we should report more specific information.

> +    virZPCIDeviceAddressPtr zpci = dev->addr.pci.zpci;
> +
> +    if (zpci && !dev->pciAddressExtFlags) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("zPCI is not supported."));
> +        return -1;
> +    }

It's called by all device types which possibly use PCI address.
I'm not sure how to report device's name in error string.




More information about the libvir-list mailing list