Re: [PATCH v3 2/6] json: Implement wrapping interface

[Daniel Kiper <dkiper@net-space.pl>]

On Wed, Nov 13, 2019 at 02:22:34PM +0100, Patrick Steinhardt wrote:
> While the newly added jsmn library provides the parsing interface, it
> does not provide any kind of interface to act on parsed tokens. Instead,
> the caller is expected to handle pointer arithmetics inside of the token
> array in order to extract required information. While simple, this
> requires users to know some of the inner workings of the library and is
> thus quite an unintuitive interface.
>
> This commit adds a new interface on top of the jsmn parser that provides
> convenience functions to retrieve values from the parsed json type,
> `grub_json_t`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/lib/json/json.c | 212 ++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h |  92 +++++++++++++++++
>  2 files changed, 304 insertions(+)
>  create mode 100644 grub-core/lib/json/json.h
>
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 2bddd8c46..ec971305a 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -17,7 +17,219 @@
>   */
>
>  #include <grub/dl.h>
> +#include <grub/mm.h>
>
> +#define JSMN_STATIC
>  #include "jsmn.h"
> +#include "json.h"
>
>  GRUB_MOD_LICENSE ("GPLv3");
> +
> +grub_err_t
> +grub_json_parse (grub_json_t **out, char *string, grub_size_t string_len)
> +{
> +  grub_json_t *json = NULL;
> +  jsmn_parser parser;
> +  grub_err_t err;
> +  int jsmn_err;
> +
> +  json = grub_zalloc (sizeof (*json));
> +  if (!json)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +  json->idx = 0;

This initialization is redundant.

> +  json->string = string;
> +  if (!json->string)
> +    {
> +      err = GRUB_ERR_OUT_OF_MEMORY;

Hmmm??? GRUB_ERR_BAD_ARGUMENT??? And please check string instead of
json->string. Additionally, if you check it just at the beginning of
the function you can do "return GRUB_ERR_BAD_ARGUMENT" immediately.

> +      goto out;
> +    }
> +
> +  jsmn_init(&parser);
> +  jsmn_err = jsmn_parse (&parser, string, string_len, NULL, 0);
> +  if (jsmn_err <= 0)
> +    {
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto out;
> +    }
> +
> +  json->tokens = grub_malloc (sizeof (jsmntok_t) * jsmn_err);
> +  if (!json->tokens)
> +    {
> +      err = GRUB_ERR_OUT_OF_MEMORY;
> +      goto out;
> +    }
> +
> +  jsmn_init(&parser);

Do you need to run jsmn_init() twice? By the way, missing space before "(".

> +  jsmn_err = jsmn_parse (&parser, string, string_len, json->tokens, jsmn_err);

Could you shortly explain before first jsmn_parse() call what are you
doing there? And maybe add a comment before this jsmn_parse() call too.

> +  if (jsmn_err <= 0)
> +    {
> +      err = GRUB_ERR_BAD_ARGUMENT;
> +      goto out;
> +    }
> +
> +  err = GRUB_ERR_NONE;

Please initialize err in the variables definition section.

> +  *out = json;

Please add an empty line here.

> +out:

Oh, out label name clashes with out argument name. Please rename out label
to err and err variable to ret. And please add a space before label name.

> +  if (err && json)
> +    {
> +      grub_free (json->string);
> +      grub_free (json->tokens);
> +      grub_free (json);
> +    }
> +  return err;
> +}
> +
> +void
> +grub_json_free (grub_json_t *json)
> +{
> +  if (json)
> +    {
> +      grub_free (json->tokens);
> +      grub_free (json);
> +    }
> +}
> +
> +grub_size_t
> +grub_json_getsize (const grub_json_t *json)
> +{
> +  jsmntok_t *p = &((jsmntok_t *)json->tokens)[json->idx];

Please add an empty line here.

> +  return p->size;
> +}
> +
> +grub_json_type_t
> +grub_json_gettype (const grub_json_t *json)
> +{
> +  jsmntok_t *p = &((jsmntok_t *)json->tokens)[json->idx];

Ditto...

> +  switch (p->type)
> +    {
> +    case JSMN_OBJECT:
> +      return GRUB_JSON_OBJECT;
> +    case JSMN_ARRAY:
> +      return GRUB_JSON_ARRAY;
> +    case JSMN_STRING:
> +      return GRUB_JSON_STRING;
> +    case JSMN_PRIMITIVE:
> +      return GRUB_JSON_PRIMITIVE;
> +    default:
> +      break;

You do not need break here.

> +    }
> +  return GRUB_JSON_UNDEFINED;
> +}
> +
> +grub_err_t
> +grub_json_getchild(grub_json_t *out, const grub_json_t *parent, grub_size_t n)
> +{
> +  jsmntok_t *p = &((jsmntok_t *)parent->tokens)[parent->idx];

Should not you check parent for NULL first? Same thing for functions
above and below.

> +  grub_size_t offset = 1;
> +
> +  if (n >= (unsigned) p->size)

Should not you cast to grub_size_t? Or should n be type of p->size?
Then you would avoid a cast.

> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  while (n--)
> +    n += p[offset++].size;
> +
> +  out->string = parent->string;
> +  out->tokens = parent->tokens;
> +  out->idx = parent->idx + offset;
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_getvalue(grub_json_t *out, const grub_json_t *parent, const char *key)
> +{
> +  grub_size_t i;
> +
> +  if (grub_json_gettype (parent) != GRUB_JSON_OBJECT)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  for (i = 0; i < grub_json_getsize (parent); i++)
> +    {
> +      grub_json_t child;
> +      const char *s;
> +
> +      if (grub_json_getchild (&child, parent, i) ||
> +	  grub_json_getstring (&s, &child, NULL) ||
> +          grub_strcmp (s, key) != 0)
> +	continue;
> +
> +      return grub_json_getchild (out, &child, 0);
> +    }
> +
> +  return GRUB_ERR_FILE_NOT_FOUND;
> +}
> +
> +static grub_err_t
> +get_value (grub_json_type_t *out_type, const char **out_string, const grub_json_t *parent, const char *key)
> +{
> +  const grub_json_t *p = parent;
> +  grub_json_t child;
> +  jsmntok_t *tok;
> +
> +  if (key)
> +    {
> +      grub_err_t err = grub_json_getvalue (&child, parent, key);

Please define ret instead of err at the beginning of the function and use
it here.

> +      if (err)
> +	return err;
> +      p = &child;
> +    }
> +
> +  tok = &((jsmntok_t *) p->tokens)[p->idx];
> +  p->string[tok->end] = '\0';
> +
> +  *out_string = p->string + tok->start;
> +  *out_type = grub_json_gettype (p);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_getstring (const char **out, const grub_json_t *parent, const char *key)
> +{
> +  grub_json_type_t type;
> +  const char *value;
> +  grub_err_t err;
> +
> +  err = get_value(&type, &value, parent, key);

Please use ret name instead of err everywhere.

> +  if (err)
> +    return err;
> +  if (type != GRUB_JSON_STRING)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  *out = value;
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_getuint64(grub_uint64_t *out, const grub_json_t *parent, const char *key)
> +{
> +  grub_json_type_t type;
> +  const char *value;
> +  grub_err_t err;

Ditto...

> +  err = get_value(&type, &value, parent, key);
> +  if (err)
> +    return err;
> +  if (type != GRUB_JSON_STRING && type != GRUB_JSON_PRIMITIVE)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  *out = grub_strtoul (value, NULL, 10);
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_getint64(grub_int64_t *out, const grub_json_t *parent, const char *key)
> +{
> +  grub_json_type_t type;
> +  const char *value;
> +  grub_err_t err;
> +
> +  err = get_value(&type, &value, parent, key);
> +  if (err)
> +    return err;
> +  if (type != GRUB_JSON_STRING && type != GRUB_JSON_PRIMITIVE)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  *out = grub_strtol (value, NULL, 10);
> +  return GRUB_ERR_NONE;
> +}
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> new file mode 100644
> index 000000000..db8160c3a
> --- /dev/null
> +++ b/grub-core/lib/json/json.h
> @@ -0,0 +1,92 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2019  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef GRUB_JSON_JSON_H
> +#define GRUB_JSON_JSON_H	1
> +
> +#include <grub/types.h>
> +
> +enum grub_json_type
> +{
> +  /* Unordered collection of key-value pairs. */
> +  GRUB_JSON_OBJECT,
> +  /* Ordered list of zero or more values. */
> +  GRUB_JSON_ARRAY,
> +  /* Zero or more Unicode characters. */
> +  GRUB_JSON_STRING,
> +  /* Number, boolean or empty value. */
> +  GRUB_JSON_PRIMITIVE,
> +  /* Invalid token. */
> +  GRUB_JSON_UNDEFINED,
> +};
> +typedef enum grub_json_type grub_json_type_t;
> +
> +struct grub_json
> +{
> +  void *tokens;
> +  char *string;
> +  grub_size_t idx;
> +};
> +typedef struct grub_json grub_json_t;
> +
> +/* Parse a JSON-encoded string. Note that the string passed to
> + * this function will get modified on subsequent calls to
> + * `grub_json_get*`. Returns the root object of the parsed JSON
> + * object, which needs to be free'd via `grub_json_free`.

Please use grub_json_free() instead of `grub_json_free` in every
comment. Same for grub_json_get*(), etc.

And more about comments in the GRUB code you can find here:
  https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments

> + */
> +grub_err_t
> +grub_json_parse (grub_json_t **out, char *string, grub_size_t string_len);

extern grub_err_t EXPORT_FUNC(grub_json_parse) (grub_json_t **out,
						char *string,
						grub_size_t string_len);

> +/* Free the structure and its contents. The string passed to
> + * `grub_json_parse` will not be free'd.
> + */
> +void
> +grub_json_free (grub_json_t *json);

extern void EXPORT_FUNC(grub_json_free) (grub_json_t *json);

...and below...

Daniel