在 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 写道:
+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
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote: [...] 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.