Re: [PATCH v3 1/2] bhyve: Separate out checks from virBhyveProbeCaps

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

 



On 11/07/2016 09:20 AM, Roman Bogorodskiy wrote:
From: Fabian Freyer <fabian.freyer@xxxxxxxxxxxxxxxxxxx>

At the moment this is just one check, but separating this out into a
separate function makes checks more modular, allowing for more readable
code once more checks are added. This also makes checks more easily
testable.

Signed-off-by: Roman Bogorodskiy <bogorodskiy@xxxxxxxxx>
---
  src/bhyve/bhyve_capabilities.c | 31 ++++++++++++++++++++++---------
  1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
index 10c33b9..be68e51 100644
--- a/src/bhyve/bhyve_capabilities.c
+++ b/src/bhyve/bhyve_capabilities.c
@@ -168,19 +168,13 @@ virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps)
      return ret;
  }
-int
-virBhyveProbeCaps(unsigned int *caps)
+static int
+bhyveProbeCapsRTC_UTC(unsigned int *caps, char *binary)
  {
-    char *binary, *help;
+    char *help;
      virCommandPtr cmd = NULL;
      int ret = 0, exit;

It's more common for functions to have ret initialized to -1, then set ret = 0 just before the "cleanup" label (if execution has gotten that far, you were successful). You can then eliminate the extra "ret = -1" on failure in this function.

(In this case, the amount of code is the same, but in functions with many failure paths, it's less lines to assume the failure value for ret. Doing it the same way in this function is just more consistent with other code.)

- binary = virFindFileInPath("bhyve");
-    if (binary == NULL)
-        goto out;
-    if (!virFileIsExecutable(binary))
-        goto out;
-
      cmd = virCommandNew(binary);
      virCommandAddArg(cmd, "-h");
      virCommandSetErrorBuffer(cmd, &help);
@@ -195,6 +189,25 @@ virBhyveProbeCaps(unsigned int *caps)
   out:

Also, the libvirt hacking guide requests that you use the name "cleanup" for a label that can be jumped to in case of an error, or dropped through in case of success. (Yes, I know there are lots of cases of "out:" in the code, but I try to get rid of them whenever I'm touching code nearby).

      VIR_FREE(help);
      virCommandFree(cmd);
+    return ret;
+}
+
+int
+virBhyveProbeCaps(unsigned int *caps)


If your path is anything like qemu's, you're eventually going to want to make caps at least a virBitmapPtr, and even more likely a virBhyveCapsPtr. (not that I'm suggesting you do that now, but the sooner you do it, the easier it will be to make the switch :-)


+{
+    char *binary;
+    int ret = 0;
+
+    binary = virFindFileInPath("bhyve");
+    if (binary == NULL)
+        goto out;
+    if (!virFileIsExecutable(binary))
+        goto out;
+
+    if ((ret = bhyveProbeCapsRTC_UTC(caps, binary)))
+        goto out;

This is technically correct, but the convention in libvirt code is to take the branch if the return value is < 0.

+
+ out:

Again, libvirt prefers the label "cleanup" instead of "out".

      VIR_FREE(binary);
      return ret;
  }


ACK, but I'd prefer you change both functions to 1) init ret = -1, 2) change the labels from out: to cleanup:, and 3) compare ret < 0 when checking for failure of bhyveProbeCapsRTC_UTC().

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[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]