在 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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
