Re: [libvirt] [PATCH v3 1/2] bhyve: Separate out checks from virBhyveProbeCaps
On 11/07/2016 09:20 AM, Roman Bogorodskiy wrote:
From: Fabian Freyer <fabian.freyer(a)physik.tu-berlin.de>
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(a)gmail.com>
---
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().