Re: [PATCH 05/11] tools: add option inactive to nodedev-dumpxml

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

 



On 2/1/24 17:39, Jonathon Jongsma wrote:
@@ -608,7 +613,15 @@ cmdNodeDeviceDumpXML(vshControl *ctl, const vshCmd *cmd)
      if (!device)
          return false;
-    if (!(xml = virNodeDeviceGetXMLDesc(device, 0)))
+    if (vshCommandOptBool(cmd, "inactive")) {
+        flags |= VIR_NODE_DEVICE_GET_XML_DESC_INACTIVE;
+        if (!virNodeDeviceIsPersistent(device)) {
+            vshError(ctl, _("Node device '%1$s' is not persistent"), device_value);
+            return false;
+        }

I believe you've already handled this within virNodeDeviceGetXMLDesc() in patch 4. There shouldn't be any need to duplicate that check here.


Without the check in the client the error message looks like this:
# virsh nodedev-dumpxml --inactive mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008 error: Requested operation is not valid: node device 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' is not persistent

I added the additional check in the virsh client to create a user friendlier message looking like this: # virsh nodedev-dumpxml --inactive mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008 error: Node device 'mdev_c60cc60c_c60c_c60c_c60c_c60cc60cc60c_0_0_0008' is not persistent

Do you want it removed?

I think the first error message is fine, and it's more maintainable to not keep the precondition checks within the function itself.


Ok, I removed the precheck.

--
Mit freundlichen Grüßen/Kind regards
   Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx




[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