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

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!