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

[Laine Stump]

On 11/07/2016 09:20 AM, Roman Bogorodskiy wrote:
> From: Fabian Freyer <fabian.freyer at 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 at 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().