Re: [libvirt] [PATCH v3 1/2] bhyve: Separate out checks from virBhyveProbeCaps
On 11/14/2016 11:45 AM, Laine Stump wrote:
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.
... and I forgot to say it before, but if you've initialized ret = -1,
then you don't want to save the return value of bhyveProbeCapsRTC_UTC()
in ret. Instead, just do this:
if (bhyveProbeCapsRTC_UTC(caps, binary) < 0)
goto cleanup;
> +
> + out:
Again, libvirt prefers the label "cleanup" instead of "out".
(and should be preceded by "ret = 0;")
> 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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list