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

[Laine Stump]

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 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.

... 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>