[PATCH libvirt v2 2/5] conf: fix zPCI address auto-generation on s390

Andrea Bolognani abologna at redhat.com
Thu Jun 25 17:43:38 UTC 2020


On Thu, 2020-06-18 at 10:25 +0200, Shalini Chellathurai Saroja wrote:
> @@ -129,7 +129,8 @@ static void
>  virDomainZPCIAddressReleaseUid(virHashTablePtr set,
>                                 virZPCIDeviceAddressPtr addr)
>  {
> -    virDomainZPCIAddressReleaseId(set, &addr->uid, "uid");
> +    if (addr->uid.isSet)
> +        virDomainZPCIAddressReleaseId(set, &addr->uid.value, "uid");
>  }
>  
> @@ -137,7 +138,8 @@ static void
>  virDomainZPCIAddressReleaseFid(virHashTablePtr set,
>                                 virZPCIDeviceAddressPtr addr)
>  {
> -    virDomainZPCIAddressReleaseId(set, &addr->fid, "fid");
> +    if (addr->fid.isSet)
> +        virDomainZPCIAddressReleaseId(set, &addr->fid.value, "fid");
>  }
> 
> -static int
> -virDomainZPCIAddressReserveNextAddr(virDomainZPCIAddressIdsPtr zpciIds,
> -                                    virZPCIDeviceAddressPtr addr)
> -{
> -    if (virDomainZPCIAddressReserveNextUid(zpciIds->uids, addr) < 0)
> -        return -1;
> -
> -    if (virDomainZPCIAddressReserveNextFid(zpciIds->fids, addr) < 0) {
> -        virDomainZPCIAddressReleaseUid(zpciIds->uids, addr);
> -        return -1;
> -    }
> +    addr->uid.isSet = true;
> +    addr->fid.isSet = true;

First of all, this is much closer to what I had in mind. Good job!

We're not quite there yet, though: as you can see from the hunks
above, there are still many scenarios in which we're either
manipulating id->value and id->isSet separately, or we needlessly
duplicate checks and don't take advantage of the fact that we now
have the virZPCIDeviceAddressID struct.

I have attached a patch that fixes these issues: as you can see,
it's pretty simple, because you've laid all the groundwork :) so it
just needs to polish things up a bit. It also solves the situation
where calling virDomainZPCIAddressRelease{U,F}id() would not set
id->isSet back to false.

Speaking of polish, while functionally this doesn't make any
difference it would be IMHO better to rename the current
virDomainZPCIAddressReserveAddr() to
virDomainZPCIAddressEnsureAddr(), since in terms of semantics it
more closely matches those of the PCI address function of the same
name. This will hopefully help reduce confusion. I've attached a
patch that performs this change as well.

Everything else looks good. Please let me know what you think of
the changes in the attached patches!

-- 
Andrea Bolognani / Red Hat / Virtualization
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-conf-Move-id-value-isSet-handling-further-down-the-s.patch
Type: text/x-patch
Size: 5675 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200625/dd7674e2/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-conf-Rename-virDomainZPCIAddress-Reserve-Ensure-Addr.patch
Type: text/x-patch
Size: 2099 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20200625/dd7674e2/attachment-0003.bin>


More information about the libvir-list mailing list