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

[Daniel Kiper]

On Fri, Nov 29, 2019 at 07:51:46AM +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 <address@hidden>
> ---
>  grub-core/lib/json/json.c | 243 ++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h | 105 ++++++++++++++++
>  2 files changed, 348 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..2093d7c98 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -17,7 +17,250 @@
>   */
>
>  #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 ret = GRUB_ERR_NONE;
> +  int jsmn_ret;
> +
> +  if (!string)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  json = grub_zalloc (sizeof (*json));
> +  if (!json)
> +    return GRUB_ERR_OUT_OF_MEMORY;
> +  json->string = string;
> +
> +  /*
> +   * Parse the string twice: first to determine how many tokens
> +   * we need to allocate, second to fill allocated tokens.
> +   */
> +  jsmn_init (&parser);
> +  jsmn_ret = jsmn_parse (&parser, string, string_len, NULL, 0);
> +  if (jsmn_ret <= 0)
> +    {
> +      ret = GRUB_ERR_BAD_ARGUMENT;
> +      goto err;
> +    }
> +
> +  json->tokens = grub_malloc (sizeof (jsmntok_t) * jsmn_ret);
> +  if (!json->tokens)
> +    {
> +      ret = GRUB_ERR_OUT_OF_MEMORY;
> +      goto err;
> +    }
> +
> +  jsmn_init (&parser);
> +  jsmn_ret = jsmn_parse (&parser, string, string_len, json->tokens, 
> jsmn_ret);
> +  if (jsmn_ret <= 0)
> +    {
> +      ret = GRUB_ERR_BAD_ARGUMENT;
> +      goto err;
> +    }
> +
> +  *out = json;
> +
> + err:
> +  if (ret && json)
> +    {
> +      grub_free (json->string);
> +      grub_free (json->tokens);
> +      grub_free (json);
> +    }
> +  return ret;
> +}
> +
> +void
> +grub_json_free (grub_json_t *json)
> +{
> +  if (json)
> +    {
> +      grub_free (json->tokens);
> +      grub_free (json);
> +    }
> +}
> +
> +grub_err_t
> +grub_json_getsize (grub_size_t *out, const grub_json_t *json)
> +{
> +  int size;

I hope that ((jsmntok_t *)json->tokens)() returns int...

> +  if (!json)
> +    return GRUB_ERR_BAD_ARGUMENT;

Hmmm... I am looking at this and I have a feeling that we are not
consistent. If we want to be consistent we should check "out" for NULL
too. Same below. However, this complicates the code needlessly. How jsmn
copes with NULL? Does it care at all? Maybe we should just drop these
checks and state clearly somewhere in docs/comments that the caller must
provide valid pointers. Full stop!

> +  size = ((jsmntok_t *)json->tokens)[json->idx].size;
> +  if (size < 0)
> +    return GRUB_ERR_BAD_ARGUMENT;

s/GRUB_ERR_BAD_ARGUMENT/GRUB_ERR_OUT_OF_RANGE/

> +  *out = (size_t) size;

s/size_t/grub_size_t/

However, TBH I would prefer grub_ssize_t here...

> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_gettype (grub_json_type_t *out, const grub_json_t *json)
> +{
> +  if (!json)
> +    return GRUB_ERR_BAD_ARGUMENT;

Ditto and below...

> +  switch (((jsmntok_t *)json->tokens)[json->idx].type)
> +    {
> +    case JSMN_OBJECT:
> +      *out = GRUB_JSON_OBJECT;
> +      break;
> +    case JSMN_ARRAY:
> +      *out = GRUB_JSON_ARRAY;
> +      break;
> +    case JSMN_STRING:
> +      *out = GRUB_JSON_STRING;
> +      break;
> +    case JSMN_PRIMITIVE:
> +      *out = GRUB_JSON_PRIMITIVE;
> +      break;
> +    default:
> +      return GRUB_ERR_BAD_ARGUMENT;
> +    }
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_getchild (grub_json_t *out, const grub_json_t *parent, grub_size_t 
> n)
> +{
> +  grub_size_t offset = 1, size;
> +  jsmntok_t *p;
> +
> +  if (grub_json_getsize(&size, parent) || n >= size)

Missing space between function name and "(".

> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  /*
> +   * Skip the first n children. For each of the children, we need
> +   * to skip their own potential children (e.g. if it's an
> +   * array), as well. We thus add the children's size to n on
> +   * each iteration.
> +   */
> +  p = &((jsmntok_t *)parent->tokens)[parent->idx];
> +  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_json_type_t type;
> +  grub_size_t i, size;
> +
> +  if (grub_json_gettype (&type, parent) || type != GRUB_JSON_OBJECT)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  if (grub_json_getsize (&size, parent))
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  for (i = 0; i < size; 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;
> +  grub_err_t ret;
> +  jsmntok_t *tok;
> +
> +  if (!parent)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  if (key)
> +    {
> +      ret = grub_json_getvalue (&child, parent, key);
> +      if (ret)
> +     return ret;
> +      p = &child;
> +    }
> +
> +  tok = &((jsmntok_t *) p->tokens)[p->idx];
> +  p->string[tok->end] = '\0';

Are you sure that tok is never NULL?

> +  *out_string = p->string + tok->start;
> +
> +  return grub_json_gettype (out_type, p);
> +}
> +
> +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 ret;
> +
> +  ret = get_value (&type, &value, parent, key);
> +  if (ret)
> +    return ret;
> +  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)

Missing space between function name and "(".

> +{
> +  grub_json_type_t type;
> +  const char *value;
> +  grub_err_t ret;
> +
> +  ret = get_value (&type, &value, parent, key);
> +  if (ret)
> +    return ret;
> +  if (type != GRUB_JSON_STRING && type != GRUB_JSON_PRIMITIVE)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  *out = grub_strtoul (value, NULL, 10);

Please check for errors here. And I would not ignore endptr. Anyway,
"man strtoul" is your friend.

> +  return GRUB_ERR_NONE;
> +}
> +
> +grub_err_t
> +grub_json_getint64(grub_int64_t *out, const grub_json_t *parent, const char 
> *key)

Ditto.

> +{
> +  grub_json_type_t type;
> +  const char *value;
> +  grub_err_t ret;
> +
> +  ret = get_value (&type, &value, parent, key);
> +  if (ret)
> +    return ret;
> +  if (type != GRUB_JSON_STRING && type != GRUB_JSON_PRIMITIVE)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  *out = grub_strtol (value, NULL, 10);

Ditto.

> +  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..48ec68e4d
> --- /dev/null
> +++ b/grub-core/lib/json/json.h
> @@ -0,0 +1,105 @@
> +/*
> + *  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,
> +
> +};

Please drop redundant comma and empty line.

Daniel