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