Re: [PATCH 5/9] util: json: Split out code to create json value objects

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 10/14/2014 03:29 AM, Peter Krempa wrote:
> Our qemu monitor code has a converter from key-value pairs to a json
> value object. I want to re-use the code later and having it part of the
> monitor command generator is inflexible. Split it out into a separate
> helper.
> ---
>  src/libvirt_private.syms     |   2 +
>  src/qemu/qemu_monitor_json.c | 161 +--------------------------------
>  src/util/virjson.c           | 211 +++++++++++++++++++++++++++++++++++++++++++
>  src/util/virjson.h           |   5 +
>  4 files changed, 220 insertions(+), 159 deletions(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index d6265ac..314b2b8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1503,6 +1503,8 @@ virJSONValueObjectAppendNumberLong;
>  virJSONValueObjectAppendNumberUint;
>  virJSONValueObjectAppendNumberUlong;
>  virJSONValueObjectAppendString;
> +virJSONValueObjectCreate;
> +virJSONValueObjectCreateVArgs;
>  virJSONValueObjectGet;
>  virJSONValueObjectGetBoolean;
>  virJSONValueObjectGetKey;
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 7f23aae..2967193 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -431,7 +431,6 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...)
>      virJSONValuePtr obj;
>      virJSONValuePtr jargs = NULL;
>      va_list args;
> -    char *key;
> 
>      va_start(args, cmdname);
> 
> @@ -442,164 +441,8 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...)
>                                         cmdname) < 0)
>          goto error;
> 
> -    while ((key = va_arg(args, char *)) != NULL) {
> -        int ret;
> -        char type;
> -
> -        if (strlen(key) < 3) {
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("argument key '%s' is too short, missing type prefix"),
> -                           key);
> -            goto error;
> -        }
> -
> -        /* Keys look like   s:name  the first letter is a type code:
> -         * Explanation of type codes:
> -         * s: string value, must be non-null
> -         * S: string value, omitted if null
> -         *
> -         * i: signed integer value
> -         * j: signed integer value, error if negative
> -         * z: signed integer value, omitted if zero
> -         * y: signed integer value, omitted if zero, error if negative
> -         *
> -         * I: signed long integer value
> -         * J: signed long integer value, error if negative
> -         * Z: signed long integer value, omitted if zero
> -         * Y: signed long integer value, omitted if zero, error if negative
> -         *
> -         * u: unsigned integer value
> -         * p: unsigned integer value, omitted if zero
> -         *
> -         * U: unsigned long integer value (see below for quirks)
> -         * P: unsigned long integer value, omitted if zero
> -         *
> -         * b: boolean value
> -         * B: boolean value, omitted if false
> -         *
> -         * d: double precision floating point number
> -         * n: json null value
> -         * a: json array
> -         */
> -        type = key[0];
> -        key += 2;
> -
> -        if (!jargs &&
> -            !(jargs = virJSONValueNewObject()))
> -            goto error;
> -
> -        /* This doesn't support maps, but no command uses those.  */
> -        switch (type) {
> -        case 'S':
> -        case 's': {
> -            char *val = va_arg(args, char *);
> -            if (!val) {
> -                if (type == 'S')
> -                    continue;
> -
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("argument key '%s' must not have null value"),
> -                               key);
> -                goto error;
> -            }
> -            ret = virJSONValueObjectAppendString(jargs, key, val);
> -        }   break;
> -
> -        case 'z':
> -        case 'y':
> -        case 'j':
> -        case 'i': {
> -            int val = va_arg(args, int);
> -
> -            if (val < 0 && (type == 'j' || type == 'y')) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("argument key '%s' must not be negative"),
> -                               key);
> -                goto error;
> -            }
> -
> -            if (!val && (type == 'z' || type == 'y'))
> -                continue;
> -
> -            ret = virJSONValueObjectAppendNumberInt(jargs, key, val);
> -        }   break;
> -
> -        case 'p':
> -        case 'u': {
> -            unsigned int val = va_arg(args, unsigned int);
> -
> -            if (!val && type == 'p')
> -                continue;
> -
> -            ret = virJSONValueObjectAppendNumberUint(jargs, key, val);
> -        }   break;
> -
> -        case 'Z':
> -        case 'Y':
> -        case 'J':
> -        case 'I': {
> -            long long val = va_arg(args, long long);
> -
> -            if (val < 0 && (type == 'J' || type == 'Y')) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("argument key '%s' must not be negative"),
> -                               key);
> -                goto error;
> -            }
> -
> -            if (!val && (type == 'Z' || type == 'Y'))
> -                continue;
> -
> -            ret = virJSONValueObjectAppendNumberLong(jargs, key, val);
> -        }   break;
> -
> -        case 'P':
> -        case 'U': {
> -            /* qemu silently truncates numbers larger than LLONG_MAX,
> -             * so passing the full range of unsigned 64 bit integers
> -             * is not safe here.  Pass them as signed 64 bit integers
> -             * instead.
> -             */
> -            long long val = va_arg(args, long long);
> -
> -            if (!val && type == 'P')
> -                continue;
> -
> -            ret = virJSONValueObjectAppendNumberLong(jargs, key, val);
> -        }   break;
> -
> -        case 'd': {
> -            double val = va_arg(args, double);
> -            ret = virJSONValueObjectAppendNumberDouble(jargs, key, val);
> -        }   break;
> -
> -        case 'B':
> -        case 'b': {
> -            int val = va_arg(args, int);
> -
> -            if (!val && type == 'B')
> -                continue;
> -
> -            ret = virJSONValueObjectAppendBoolean(jargs, key, val);
> -        }   break;
> -
> -        case 'n': {
> -            ret = virJSONValueObjectAppendNull(jargs, key);
> -        }   break;
> -
> -        case 'a': {
> -            virJSONValuePtr val = va_arg(args, virJSONValuePtr);
> -            ret = virJSONValueObjectAppend(jargs, key, val);
> -        }   break;
> -
> -        default:
> -            virReportError(VIR_ERR_INTERNAL_ERROR,
> -                           _("unsupported data type '%c' for arg '%s'"), type, key - 2);
> -            goto error;
> -        }
> -        if (ret < 0)
> -            goto error;
> -    }
> +    if (virJSONValueObjectCreateVArgs(&jargs, args) < 0)
> +        goto error;
> 
>      if (jargs &&
>          virJSONValueObjectAppend(obj, wrap ? "data" : "arguments", jargs) < 0)
> diff --git a/src/util/virjson.c b/src/util/virjson.c
> index ec83b2f..0dfeaed 100644
> --- a/src/util/virjson.c
> +++ b/src/util/virjson.c
> @@ -63,6 +63,217 @@ struct _virJSONParser {
>  };
> 
> 
> +/**
> + * virJSONValueObjectCreateVArgs:
> + * @obj: returns the created JSON object
> + * @...: a key-value argument pairs, terminated by NULL
> + *
> + * Creates a JSON value object filled with key-value pairs supplied as variable
> + * argument list.
> + *
> + * Keys look like   s:name  the first letter is a type code:
> + * Explanation of type codes:
> + * s: string value, must be non-null
> + * S: string value, omitted if null
> + *
> + * i: signed integer value
> + * j: signed integer value, error if negative
> + * z: signed integer value, omitted if zero
> + * y: signed integer value, omitted if zero, error if negative
> + *
> + * I: signed long integer value
> + * J: signed long integer value, error if negative
> + * Z: signed long integer value, omitted if zero
> + * Y: signed long integer value, omitted if zero, error if negative
> + *
> + * u: unsigned integer value
> + * p: unsigned integer value, omitted if zero
> + *
> + * U: unsigned long integer value (see below for quirks)
> + * P: unsigned long integer value, omitted if zero
> + *
> + * b: boolean value
> + * B: boolean value, omitted if false
> + *
> + * d: double precision floating point number
> + * n: json null value
> + * a: json array
> + *
> + * The value corresponds to the selected type.
> + *
> + * Returns -1 on error. 1 on success, if at least one key:pair was valid 0
> + * in case of no error but nothing was filled (@obj will be NULL).
> + */
> +int
> +virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args)
> +{
> +    virJSONValuePtr jargs = NULL;
> +    char type;
> +    char *key;
> +    int ret = -1;
> +    int rc = -2; /* -2 is a sentinel to check if at least one entry was added */
> +
> +    *obj = NULL;
> +
> +    if (!(jargs = virJSONValueNewObject()))
> +        goto cleanup;
> +
> +    while ((key = va_arg(args, char *)) != NULL) {
> +
> +        if (strlen(key) < 3) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("argument key '%s' is too short, missing type prefix"),
> +                           key);
> +            goto cleanup;
> +        }
> +
> +        type = key[0];
> +        key += 2;
> +
> +        /* This doesn't support maps, but no command uses those.  */
> +        switch (type) {
> +        case 'S':
> +        case 's': {
> +            char *val = va_arg(args, char *);
> +            if (!val) {
> +                if (type == 'S')
> +                    continue;
> +
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("argument key '%s' must not have null value"),
> +                               key);
> +                goto cleanup;
> +            }
> +            rc = virJSONValueObjectAppendString(jargs, key, val);
> +        }   break;
> +
> +        case 'z':
> +        case 'y':
> +        case 'j':
> +        case 'i': {
> +            int val = va_arg(args, int);
> +
> +            if (val < 0 && (type == 'j' || type == 'y')) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("argument key '%s' must not be negative"),
> +                               key);
> +                goto cleanup;
> +            }
> +
> +            if (!val && (type == 'z' || type == 'y'))
> +                continue;
> +
> +            rc = virJSONValueObjectAppendNumberInt(jargs, key, val);
> +        }   break;
> +
> +        case 'p':
> +        case 'u': {
> +            unsigned int val = va_arg(args, unsigned int);
> +
> +            if (!val && type == 'p')
> +                continue;
> +
> +            rc = virJSONValueObjectAppendNumberUint(jargs, key, val);
> +        }   break;
> +
> +        case 'Z':
> +        case 'Y':
> +        case 'J':
> +        case 'I': {
> +            long long val = va_arg(args, long long);
> +
> +            if (val < 0 && (type == 'J' || type == 'Y')) {
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("argument key '%s' must not be negative"),
> +                               key);
> +                goto cleanup;
> +            }
> +
> +            if (!val && (type == 'Z' || type == 'Y'))
> +                continue;
> +
> +            rc = virJSONValueObjectAppendNumberLong(jargs, key, val);
> +        }   break;
> +
> +        case 'P':
> +        case 'U': {
> +            /* qemu silently truncates numbers larger than LLONG_MAX,
> +             * so passing the full range of unsigned 64 bit integers
> +             * is not safe here.  Pass them as signed 64 bit integers
> +             * instead.
> +             */
> +            long long val = va_arg(args, long long);
> +
> +            if (!val && type == 'P')
> +                continue;
> +
> +            rc = virJSONValueObjectAppendNumberLong(jargs, key, val);
> +        }   break;
> +
> +        case 'd': {
> +            double val = va_arg(args, double);
> +            rc = virJSONValueObjectAppendNumberDouble(jargs, key, val);
> +        }   break;
> +
> +        case 'B':
> +        case 'b': {
> +            int val = va_arg(args, int);
> +
> +            if (!val && type == 'B')
> +                continue;
> +
> +            rc = virJSONValueObjectAppendBoolean(jargs, key, val);
> +        }   break;
> +
> +        case 'n': {
> +            rc = virJSONValueObjectAppendNull(jargs, key);
> +        }   break;
> +
> +        case 'a': {
> +            virJSONValuePtr val = va_arg(args, virJSONValuePtr);
> +            rc = virJSONValueObjectAppend(jargs, key, val);
> +        }   break;
> +
> +        default:
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("unsupported data type '%c' for arg '%s'"), type, key - 2);
> +            goto cleanup;
> +        }
> +
> +        if (rc < 0)
> +            goto cleanup;
> +    }
> +
> +    /* verify that we added at least one key-value pair */
> +    if (rc == -2) {

I didn't check any of the virJSAONValue* functions called, but if any
return -2 for whatever reason "sometime" in the future, then this logic
is flawed.

However, at this point you should have an array size/count right? And if
> 0, then we added something (eg, npairs count should be incremented)
thus virJSONValueObjectKeysNumber() should return > 0

> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    *obj = jargs;
> +    jargs = NULL;
> +    ret = 1;
> +
> + cleanup:
> +    virJSONValueFree(jargs);
> +    return ret;
> +}
> +
> +
> +int
> +virJSONValueObjectCreate(virJSONValuePtr *obj, ...)
> +{
> +    va_list args;
> +    int ret;
> +
> +    va_start(args, obj);
> +    ret = virJSONValueObjectCreateVArgs(obj, args);
> +    va_end(args);
> +
> +    return ret;
> +}
> +
> +
>  void
>  virJSONValueFree(virJSONValuePtr value)
>  {
> diff --git a/src/util/virjson.h b/src/util/virjson.h
> index 9487729..be603ae 100644
> --- a/src/util/virjson.h
> +++ b/src/util/virjson.h
> @@ -26,6 +26,8 @@
> 
>  # include "internal.h"
> 
> +# include <stdarg.h>
> +
> 
>  typedef enum {
>      VIR_JSON_TYPE_OBJECT,
> @@ -79,6 +81,9 @@ struct _virJSONValue {
> 
>  void virJSONValueFree(virJSONValuePtr value);
> 
> +int virJSONValueObjectCreate(virJSONValuePtr *obj, ...);
> +int virJSONValueObjectCreateVArgs(virJSONValuePtr *obj, va_list args);

Add an ATTRIBUTE_NONNULL(1) for this definition, since you deref in
function.


ACK with adjustments

John

> +
>  virJSONValuePtr virJSONValueNewString(const char *data);
>  virJSONValuePtr virJSONValueNewStringLen(const char *data, size_t length);
>  virJSONValuePtr virJSONValueNewNumberInt(int data);
> 

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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]