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. [...]
+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. I don't think that's what we want. Also all functions that return an int should be called like if (virDomainZPCIAddressReserveNextUid() < 0) { ... } [...]
+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.
+ 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. -- Andrea Bolognani / Red Hat / Virtualization