On 6/25/20 7:43 PM, Andrea Bolognani wrote:
> 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!
>
Hi Andrea,
I agree that it looks nice and sparkling. :D Thanks.
Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294