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.