On Tue, 2018-09-04 at 16:39 +0800, Yi Min Zhao wrote:
[...]
> If the user
> define zPCI extension address but zPCI capability doesn't exist, an
> error will be reported.
You're (no longer) checking for the capability here, so the commit
message should be updated accordingly.
[...]
> +bool
> +virDeviceInfoPCIAddressExtensionIsPresent(const virDomainDeviceInfo *info)
> +{
> + return info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> + !virZPCIDeviceAddressIsEmpty(&info->addr.pci.zpci);
The second line is not aligned properly.
Actually, you'll probably want to restructure this a bit so that it
only looks into info->addr.pci.zpci if the zPCI extension flag is
present, after which the comment above will likely no longer apply.
[...]
> +static int
> +virDomainZPCIAddressReserveId(virHashTablePtr set,
> + unsigned int id,
> + const char *name)
> +{
> + if (virHashLookup(set, &id)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("zPCI %s %u is already reserved"),
> + name, id);
Please format the id as octal for easier debugging when something
goes wrong. Same below.
[...]
> +static void
> +virDomainZPCIAddressReleaseUid(virHashTablePtr set,
> + virZPCIDeviceAddressPtr addr)
> +{
> + if (virHashRemoveEntry(set, &addr->uid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Release uid %u failed"), addr->uid);
> + }
You should have a generic virDomainZPCIAddressReleaseId() function
that you call from ReleaseUid() and ReleaseFid(), just like you
have for reserving them.
Additionally, it looks like failure to release an id is not a
fatal error since processing continue, so you should use VIR_WARN()
or something like that instead of virReportError() when that
happens.
[...]
> +int
> +virDomainPCIAddressExtensionReserveNextAddr(virDomainPCIAddressSetPtr addrs,
> + virPCIDeviceAddressPtr dev,
> + virDomainPCIAddressExtensionFlags extFlags)
> +{
> + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) &&
> + virZPCIDeviceAddressIsEmpty(&dev->zpci)) {
You shouldn't need the second check: just go ahead and reserve the
next address regardless of what's currently stored in the device
info, no?
[...]
> +void
> +virDomainPCIAddressExtensionReleaseAddr(virDomainPCIAddressSetPtr addrs,
> + virPCIDeviceAddressPtr addr,
> + int extFlags)
> +{
> + if ((extFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI))
You don't need two sets of parentheses here :)
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
