Re: [PATCH libvirt v1 1/6] conf: fix ZPCI address validation on s390

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

 



Hi Andrea,

Thank you for the review.

On 6/3/20 12:09 PM, Andrea Bolognani wrote:
On Thu, 2020-04-09 at 12:31 +0200, Shalini Chellathurai Saroja wrote:
-    if (uid &&
-        virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                       _("Cannot parse <address> 'uid' attribute"));
-        goto cleanup;
+    if (uid) {
+        if (virStrToLong_uip(uid, NULL, 0, &def.uid) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("Cannot parse <address> 'uid' attribute"));
+            return -1;
+        }
+        if (!virZPCIDeviceAddressIsValid(&def))
+            return -1;
      }
This doesn't seem right.

I understand that you're moving the call to
virZPCIDeviceAddressIsValid() inside the if(uid) branch so that you
won't get an error for something like

   <zpci fid='0x00000005'/>

but this is not a good way to do it: in fact, you change it again in
patch 3/6 and adopt the much better approach of storing directly in
the struct whether uid and fid have been set.

You should go for that approach right away instead of implementing
this intermediate solution first.
ok, I will do it.

- cleanup:
-    VIR_FREE(uid);
-    VIR_FREE(fid);
-    return ret;
+    return 0;
Switching to g_autofree is a nice improvement, but it's a completely
independent one so please make that a separate patch that doesn't
touch the logic.
ok, I will make this as a separate patch.





[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