Re: [libvirt] [PATCH v3 08/12] conf: Allocate/release 'uid' and 'fid' in PCI address
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