[systemd-devel] [PATCH 2/5] shared/json: JSON parser + number tokenizer bugfix

[Lennart Poettering]

On Thu, 07.05.15 17:47, Pavel Odvody (podvody at redhat.com) wrote:

> Signed-off-by: Pavel Odvody <podvody at redhat.com>

Hmm, no S-o-b (as mentioned elsewhere already).

Also, please add a commit msg explaining the need for the object parser...

> +int json_variant_new(json_variant **ret, int type) {
> +        json_variant *v;
> +        assert(!*ret);

So far all our functions returning problems would make no assumptions
about whether the pointer we shall fill in is already set or not, we'd
unconcditionally override it on success, and never touch it on
failure...


> +        v = new0(json_variant, 1);
> +        if (!v)
> +                return -ENOMEM;
> +        v->type = type;
> +        v->size = 0;
> +        v->obj  = NULL;

Hmm, the latter is a variant, so it might be surprising that you only
initialize thhis one part of it... Can't we remove the initialization
of this entirely? You use new0() after all, hence everything is
initialized to 0 anyway....

> +        *ret = v;
> +        return 0;
> +}
> +
> +static int json_variant_deep_copy(json_variant *ret, json_variant *variant) {
> +        assert(ret);
> +        assert(variant);
> +
> +        ret->type = variant->type;
> +        ret->size = variant->size;
> +
> +        if (variant->type == JSON_VARIANT_STRING) {
> +                ret->string = strndup(variant->string, variant->size);
> +                if (!ret->string)
> +                        return -ENOMEM;

Should this really be strndup() rather than memdup()?

I mean, either we care for the size or for the terminating NUL... it
appears weird here care for both. Especially if you then initialize
the size parameter to something possibly much smaller than the
string...

>  
> +enum {
> +        JSON_VARIANT_CONTROL,
> +        JSON_VARIANT_STRING,
> +        JSON_VARIANT_INTEGER,
> +        JSON_VARIANT_BOOLEAN,
> +        JSON_VARIANT_REAL,
> +        JSON_VARIANT_ARRAY,
> +        JSON_VARIANT_OBJECT,
> +        JSON_VARIANT_NULL
> +};

Why an anonmymous enum here? Shouldn't this have a properly named type?

> +
>  union json_value {
>          bool boolean;
>          double real;
>          intmax_t integer;
>  };
>  
> +typedef struct json_variant {
> +        union {
> +                char *string;
> +                struct json_variant *obj;

why abbreviate this? We try to avoid unnecessary abbreviations generally...

> +                union json_value value;
> +        };
> +        int type;
> +        unsigned size;
> +} json_variant;

I figure the struct/typedef should be name JsonVariant, as we tend to
use CamelCase for object-like types...

Lennart

-- 
Lennart Poettering, Red Hat