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 voidvirDomainZPCIAddressReleaseFid(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@xxxxxxxxxxxxx> -- 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
