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

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Sat, May 26, 2018 at 11:00:27PM +0200, Fabiano Fidêncio wrote:
> From: Fabiano Fidêncio <fidencio@xxxxxxxxxx>
>
> 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?

> - 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@xxxxxxxxxxxx>
> ---
> 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;

>     *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;

>     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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list