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

[Fabiano Fidêncio]

On Sun, May 27, 2018 at 1:17 PM, Ján Tomko <jtomko at redhat.com> wrote:
> On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
>>
>> From: Fabiano Fidêncio <fidencio at redhat.com>
>>
>> There are still some places using virConfGetValue() and then checking
>> the specific type of the pointers and so on.
>>
>> Those place are not going to be changed as:
>> - Directly using virConfGetValue*() would trigger virReportError() on
>> their current code
>
>
> Is that a problem in xenParseCPUFeatures?

It would, at least, generate one more log, which would be misleading
whoever ends up debugging some issue on that codepath later on.

>
>> - Expanding virConfGetValue*() to support strings as other types (as
>> boolean or long) doesn't seem to be the safest path to take.
>>
>> Signed-off-by: Fabiano Fidêncio <fabiano at fidencio.org>
>> ---
>> src/xenconfig/xen_common.c | 637
>> ++++++++++++++++++++++-----------------------
>> 1 file changed, 307 insertions(+), 330 deletions(-)
>>
>> diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c
>> index a2b0708ee3..2e8e95f720 100644
>> --- a/src/xenconfig/xen_common.c
>> +++ b/src/xenconfig/xen_common.c
>> @@ -145,31 +145,18 @@ xenConfigCopyStringInternal(virConfPtr conf,
>>                             char **value,
>>                             int allowMissing)
>> {
>> -    virConfValuePtr val;
>> +    int result;
>
>
> int rc;
>

Noted!

>
>>
>>     *value = NULL;
>> -    if (!(val = virConfGetValue(conf, name))) {
>> -        if (allowMissing)
>> -            return 0;
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("config value %s was missing"), name);
>> -        return -1;
>> -    }
>>
>> -    if (val->type != VIR_CONF_STRING) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("config value %s was not a string"), name);
>> -        return -1;
>> -    }
>> -    if (!val->str) {
>> -        if (allowMissing)
>> -            return 0;
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("config value %s was missing"), name);
>> -        return -1;
>> -    }
>> +    result = virConfGetValueString(conf, name, value);
>> +    if (result == 1 && *value != NULL)
>> +        return 0;
>> +
>> +    if (allowMissing)
>> +        return 0;
>>
>> -    return VIR_STRDUP(*value, val->str);
>> +    return -1;
>> }
>>
>>
>> @@ -193,7 +180,8 @@ xenConfigCopyStringOpt(virConfPtr conf, const char
>> *name, char **value)
>> static int
>> xenConfigGetUUID(virConfPtr conf, const char *name, unsigned char *uuid)
>> {
>> -    virConfValuePtr val;
>> +    char *string = NULL;
>> +    int result;
>
>
> int ret = -1;
>

Noted!

>
>>
>>     if (!uuid || !name || !conf) {
>>         virReportError(VIR_ERR_INVALID_ARG, "%s",
>> @@ -201,35 +189,35 @@ xenConfigGetUUID(virConfPtr conf, const char *name,
>> unsigned char *uuid)
>>         return -1;
>>     }
>>
>> -    if (!(val = virConfGetValue(conf, name))) {
>> -        if (virUUIDGenerate(uuid) < 0) {
>> -            virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                           "%s", _("Failed to generate UUID"));
>> -            return -1;
>> -        } else {
>> -            return 0;
>> -        }
>> -    }
>> -
>> -    if (val->type != VIR_CONF_STRING) {
>> -        virReportError(VIR_ERR_CONF_SYNTAX,
>> -                       _("config value %s not a string"), name);
>> +    if (virConfGetValueString(conf, name, &string) < 0)
>>         return -1;
>> -    }
>>
>> -    if (!val->str) {
>> +    if (!string) {
>>         virReportError(VIR_ERR_CONF_SYNTAX,
>>                        _("%s can't be empty"), name);
>>         return -1;
>>     }
>>
>> -    if (virUUIDParse(val->str, uuid) < 0) {
>> +    if (virUUIDGenerate(uuid) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s", _("Failed to generate UUID"));
>
>
>> +        result = -1;
>
>
> Redundant if you initialize it to -1;
>
>> +        goto cleanup;
>> +    }
>> +
>> +    if (virUUIDParse(string, uuid) < 0) {
>>         virReportError(VIR_ERR_CONF_SYNTAX,
>> -                       _("%s not parseable"), val->str);
>> -        return -1;
>> +                       _("%s not parseable"), string);
>
>
>> +        result = -1;
>
>
> Dtto.
>
>> +        goto cleanup;
>>     }
>>
>> -    return 0;
>> +    result = 1;
>
>
> ret = 0;
>
>> +
>> + cleanup:
>> +    VIR_FREE(string);
>
>
>> +
>
>
> Dropping this line could improve readability - it's already visually
> separated by the cleanup label and }
>
>> +    return result;
>> }
>>
>>
>> @@ -392,93 +372,92 @@ xenParseEventsActions(virConfPtr conf,
>> virDomainDefPtr def)
>> static int
>> xenParsePCI(virConfPtr conf, virDomainDefPtr def)
>> {
>> -    virConfValuePtr list = virConfGetValue(conf, "pci");
>>     virDomainHostdevDefPtr hostdev = NULL;
>> +    char **pcis = NULL, **entries;
>>
>
> Another function that would benefit from being split.
>
> [...]
>
>>
>> +    virStringListFree(pcis);
>>     return 0;
>> +
>> + cleanup:
>
>
> Use the ret variable to join these cleanup paths into one.
>
>> +    virStringListFree(pcis);
>> +    return -1;
>> }
>>
>>
>
> Jano


Thanks for the review!
-- 
Fabiano Fidêncio