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

[Max Tottenham]

On 11/05, 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 | 218 ++++++++++++++++++++++++++++++++++++++
>  include/grub/json.h       |  69 ++++++++++++
>  2 files changed, 287 insertions(+)
>  create mode 100644 include/grub/json.h
> 
> diff --git a/grub-core/lib/json/json.c b/grub-core/lib/json/json.c
> index 2bddd8c46..6a6da8fd8 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -17,7 +17,225 @@
>   */
>  
>  #include <grub/dl.h>
> +#include <grub/json.h>
> +#include <grub/mm.h>
>  
> +#define JSMN_STATIC
>  #include "jsmn.h"
>  
>  GRUB_MOD_LICENSE ("GPLv3");
> +
> +grub_err_t
> +grub_json_parse (grub_json_t **out, const char *string, grub_size_t 
> string_len)
> +{
> +  grub_size_t ntokens = 128;
> +  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;
> +  json->string = grub_strndup (string, string_len);

I'm assuming the idea here is to ensure that the lifetime of the
returned grub_json_t doesn't depend on the lifetime of the input string?

This concerns me a little bit, from a quick scan - given your usage of
grub_json_parse in the luks2 module it appears that json_header (the
underlying input string) will always outlive the json object.

In which case, as there are no other existing users, we may want to make
the API contract "The lifetime/validity of the returned object is bound
by that of the input string", if we can comment/document that clearly
then there is no need for this extra allocation and we can simply do:

    json->string = string;


Thoughts?

> +  if (!json->string)
> +    {
> +      err = GRUB_ERR_OUT_OF_MEMORY;
> +      goto out;
> +    }
> +
> +  jsmn_init(&parser);
> +
> +  while (1)
> +    {
> +      json->tokens = grub_realloc (json->tokens, sizeof (jsmntok_t) * 
> ntokens);

According to the docs, calling jsmn_parse with tokens = NULL will return
the number of tokens required to properly parse the JSON, without trying
to 'allocate' them in the token buffer (with the 'ntokens' parameter
being ignored)

Therefore I think this entire loop could be replaced with something
like:
    ...
    // Get number of tokens to allocate
    int num_tokens = jsmn_parse (&parser, string, string_len, NULL, NULL);
    json->tokens = grub_zalloc (json->tokens, sizeof (jsmntok_t) * ntokens);
    if (!json->tokens)
      {
        err = GRUB_ERR_OUT_OF_MEMORY;
        goto out;
      }
    jsmn_err = jsmn_parse (&parser, string, string_len, json->tokens,
                           num_tokens);
    if (jsmn_err < 0)
      {
        err = GRUB_ERR_BAD_ARGUMENT;
        goto out;
      }
    err = GRUB_ERR_NONE;
    *out = json;
    out:
    ...



> +      if (!json->tokens)
> +     {
> +       err = GRUB_ERR_OUT_OF_MEMORY;
> +       goto out;
> +     }
> +
> +      jsmn_err = jsmn_parse (&parser, string, string_len, json->tokens, 
> ntokens);
> +      if (jsmn_err >= 0)
> +     break;
> +      if (jsmn_err != JSMN_ERROR_NOMEM)
> +     {
> +       err = GRUB_ERR_BAD_ARGUMENT;
> +       goto out;
> +     }
> +
> +      ntokens <<= 1;
> +    }
> +
> +  err = GRUB_ERR_NONE;
> +  *out = json;
> +out:
> +  if (err && json)
> +    {
> +      grub_free (json->string);
> +      grub_free (json->tokens);

Irrespective of the above - these two free's on
json->string/json->tokens could be called on a NULL agrgment (e.g. if
their initial allocation fails), I don't know whether it's common
practice to allow grub_free (NULL), but I would encourage checking
whether these are actually allocated before attempting to free them.

> +      grub_free (json);
> +    }
> +  return err;
> +}
> +
> +void
> +grub_json_free (grub_json_t *json)
> +{
> +  if (json)
> +    {
> +      grub_free (json->string);
> +      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];
> +  return p->size;
> +}
> +
> +grub_json_type_t
> +grub_json_gettype (const grub_json_t *json)
> +{
> +  jsmntok_t *p = &((jsmntok_t *)json->tokens)[json->idx];
> +  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;
> +    }
> +  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];
> +  grub_size_t offset = 1;
> +
> +  if (n >= (unsigned) p->size)
> +    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;
> +
> +      out->string = child.string;
> +      out->tokens = child.tokens;
> +      out->idx = child.idx + 1;
> +
> +      return GRUB_ERR_NONE;
> +    }
> +
> +  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);
> +      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);
> +  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;
> +
> +  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/include/grub/json.h b/include/grub/json.h
> new file mode 100644
> index 000000000..a59f0393d
> --- /dev/null
> +++ b/include/grub/json.h
> @@ -0,0 +1,69 @@
> +/*
> + *  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
> +{
> +  GRUB_JSON_OBJECT,
> +  GRUB_JSON_ARRAY,
> +  GRUB_JSON_STRING,
> +  GRUB_JSON_PRIMITIVE,
> +  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;
> +
> +grub_err_t
> +grub_json_parse (grub_json_t **out, const char *string, grub_size_t 
> string_len);
> +
> +void
> +grub_json_free (grub_json_t *json);
> +
> +grub_size_t
> +grub_json_getsize (const grub_json_t *json);
> +
> +grub_json_type_t
> +grub_json_gettype (const grub_json_t *json);
> +
> +grub_err_t
> +grub_json_getchild (grub_json_t *out, const grub_json_t *parent, grub_size_t 
> n);
> +
> +grub_err_t
> +grub_json_getvalue (grub_json_t *out, const grub_json_t *parent, const char 
> *key);
> +
> +grub_err_t
> +grub_json_getstring (const char **out, const grub_json_t *parent, const char 
> *key);
> +
> +grub_err_t
> +grub_json_getuint64 (grub_uint64_t *out, const grub_json_t *parent, const 
> char *key);
> +
> +grub_err_t
> +grub_json_getint64 (grub_int64_t *out, const grub_json_t *parent, const char 
> *key);
> +
> +#endif
> -- 
> 2.23.0
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel

-- 
Max Tottenham       | address@hidden
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies