[libvirt] [PATCHv2 2/3] vmx: convert to typesafe virConf accessors

[Fabiano Fidêncio]

On Sun, May 27, 2018 at 1:02 PM, Ján Tomko <jtomko at redhat.com> wrote:
> On Sat, May 26, 2018 at 11:00:26PM +0200, Fabiano Fidêncio wrote:
>>
>> From: Fabiano Fidêncio <fidencio at redhat.com>
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano at fidencio.org>
>> ---
>> src/vmx/vmx.c | 196
>> ++++++++++++++++++++++------------------------------------
>> 1 file changed, 73 insertions(+), 123 deletions(-)
>>
>> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
>> index df6a58a474..54542c29a6 100644
>> --- a/src/vmx/vmx.c
>> +++ b/src/vmx/vmx.c
>> @@ -808,47 +786,35 @@ static int
>> virVMXGetConfigLong(virConfPtr conf, const char *name, long long *number,
>>                     long long default_, bool optional)
>> {
>> -    virConfValuePtr value;
>> +    char *string = NULL;
>> +    int result;
>
>
> Usually we use 'ret' as the name of the variable that will eventually be
> used to return from the function and 'rc' to store the value returned by
> the function we call (where they return more values than 0/-1):
>
> int ret = -1;
> int rc;
>

Right!

>>
>>     *number = default_;
>> -    value = virConfGetValue(conf, name);
>>
>> -    if (value == NULL) {
>> -        if (optional) {
>> -            return 0;
>> -        } else {
>> -            virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                           _("Missing essential config entry '%s'"),
>> name);
>> -            return -1;
>> -        }
>> -    }
>> +    result = virVMXGetConfigStringHelper(conf, name, &string, optional);
>> +    if (result <= 0)
>> +        return result;
>
>
> This would become:
> rc = GetStringHelper();
> if (rc <= 0)
>    return rc;
>

Okay.

>>
>> -    if (value->type == VIR_CONF_STRING) {
>> -        if (value->str == NULL) {
>> -            if (optional) {
>> -                return 0;
>> -            } else {
>> -                virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                               _("Missing essential config entry '%s'"),
>> name);
>> -                return -1;
>> -            }
>> -        }
>> +    if (STRCASEEQ(string, "unlimited")) {
>> +        *number = -1;
>> +        result = 0;
>> +        goto cleanup;
>> +    }
>>
>> -        if (STRCASEEQ(value->str, "unlimited")) {
>> -            *number = -1;
>> -        } else if (virStrToLong_ll(value->str, NULL, 10, number) < 0) {
>> -            virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                           _("Config entry '%s' must represent an integer
>> value"),
>> -                           name);
>> -            return -1;
>> -        }
>> -    } else {
>
>
> if you leave this 'else' here, there's no need to touch 'ret' or goto
> cleanup above.

Okay, I'll keep it as it is.

>
>> +    if (virStrToLong_ll(string, NULL, 10, number) < 0) {
>>         virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("Config entry '%s' must be a string"), name);
>> -        return -1;
>> +                _("Config entry '%s' must represent an integer value"),
>> +                name);
>
>
>> +        result = -1;
>
>
> This adjustment would also be unnecessary.
>
>> +        goto cleanup;
>>     }
>>
>> -    return 0;
>> +    result = 0;
>> +
>
>
> (NB: while many of the functions in this file use 'result' instead of
> 'ret', I'd suggest to slowly start replacing the old name instead of
> striving for consistency. Most of them are the Parse functions that
> already return a device definition via a pointer argument and the
> 0/-1 they return is redundant, because it can be replaced by a NULL
> check on the pointer)

Right!

>
> Jano
>
> Jano


Thanks for the review!
-- 
Fabiano Fidêncio