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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux