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

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

[Max Tottenham]

[-- Attachment #1: Type: text/plain, Size: 226 bytes --]

I got a message from my MTA indicating my original mail may have been
dropped. Attempting to resend.

-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

[-- Attachment #2: Type: message/rfc822, Size: 11375 bytes --]

From: Max Tottenham <mtottenh@akamai.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Cc: Patrick Steinhardt <ps@pks.im>
Subject: Re: [PATCH v2 2/6] json: Implement wrapping interface
Date: Tue, 5 Nov 2019 09:54:50 +0000
Message-ID: <20191105095449.GA51509@lon-mpk47.lan>

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 <ps@pks.im>
> ---
>  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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3737 bytes --]

On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> On 11/05, Patrick Steinhardt wrote:
> > +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?

Thing is that the parser needs to modify the string. This is kind
of an optimization in order to avoid the need for calling
`strdup` when returning a string, e.g. in `grub_json_getstring`.
We just modify the underlying string to have a '\0' in the right
place and then return it directly. It might feel weird to callers
that the parsed string is getting modified under their hands,
which is why I decided to just `strdup` it.

That being said, I don't have any hard feelings about this. I'm
fine with avoiding the `grub_strndup`, and if it's documented so
that callers know that it's getting modified then it should be
safe enough.

> > +  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)

jsmn's examples curiously have the same realloc-loop that I do.
I'll check again and definitely convert to what you propose if
supported.

> > +      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.

free(3P) explicitly mandates that it may be called with a `NULL`
pointer as argument, in which case it will just do nothing.
`grub_free` is in fact doing the same, so we should be good here.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Kiper]

On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote:
> On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> > On 11/05, Patrick Steinhardt wrote:
> > > +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?
>
> Thing is that the parser needs to modify the string. This is kind
> of an optimization in order to avoid the need for calling
> `strdup` when returning a string, e.g. in `grub_json_getstring`.
> We just modify the underlying string to have a '\0' in the right
> place and then return it directly. It might feel weird to callers
> that the parsed string is getting modified under their hands,
> which is why I decided to just `strdup` it.
>
> That being said, I don't have any hard feelings about this. I'm
> fine with avoiding the `grub_strndup`, and if it's documented so
> that callers know that it's getting modified then it should be
> safe enough.
>
> > > +  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)
>
> jsmn's examples curiously have the same realloc-loop that I do.
> I'll check again and definitely convert to what you propose if
> supported.

In general if allocation functions are not called thousands times and
do not alloc tons of memory then I think that data duplication gives
us more flexibility here.

> > > +      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.
>
> free(3P) explicitly mandates that it may be called with a `NULL`
> pointer as argument, in which case it will just do nothing.
> `grub_free` is in fact doing the same, so we should be good here.

Yeah, that is correct/acceptable free()/grub_free() usage.

Daniel

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

[Max Tottenham]

On 11/06, Daniel Kiper wrote:
> On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote:
> > On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> > > On 11/05, Patrick Steinhardt wrote:
> > > > +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?
> >
> > Thing is that the parser needs to modify the string. This is kind
> > of an optimization in order to avoid the need for calling
> > `strdup` when returning a string, e.g. in `grub_json_getstring`.
> > We just modify the underlying string to have a '\0' in the right
> > place and then return it directly. It might feel weird to callers
> > that the parsed string is getting modified under their hands,
> > which is why I decided to just `strdup` it.

Hmm I see, I suppose one final alternative would be to create a more
difficult to use API that requires the caller to provide the memory for
the resulting string (which would involve providing an interface to
supply information about the length of memory required).

Something like:

    char buff[DEFAULT_LEN] = { 0 };
    char *type;
    grub_err_t err = grub_json_getstring(NULL, keyslot, "type", &size);
    type = size < DEFAULT_LEN ? buff : grub_zmalloc(size);
    grub_json_getstring(type, keyslot "type", &size);
    ...

Although now that I mention that it seems like it's just a pain to get
right/use, so I think we can probably just discount that one out of
hand.
    


> >
> > That being said, I don't have any hard feelings about this. I'm
> > fine with avoiding the `grub_strndup`, and if it's documented so
> > that callers know that it's getting modified then it should be
> > safe enough.
> >

My personal thought is the fewer copies/allocations we do in GRUB the
better, I think It's reasonable to have the API consume the input string
while parsing it as long as it's well documented, and to put the onus on
the caller to provide a copy of the input string should they want to
keep the original around for some reason.


> > > > +  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)
> >
> > jsmn's examples curiously have the same realloc-loop that I do.
> > I'll check again and definitely convert to what you propose if
> > supported.
> 

I think that's because they have only two examples, one that expects a
maximum of 128 tokens and doesn't bother performing any dynamic
allocation, and another that reads from stdin in chunks - so the
input string length isn't known in advance.

In our case, we know the total input string size ahead of time (as it's
embedded in the binary header).

> In general if allocation functions are not called thousands times and
> do not alloc tons of memory then I think that data duplication gives
> us more flexibility here.
> 

I'm not sure it does, I suppose there is the case where we'd want to do
a debug dump of the string? But to do that you could simply take a copy
before parsing.


> >
> > free(3P) explicitly mandates that it may be called with a `NULL`
> > pointer as argument, in which case it will just do nothing.
> > `grub_free` is in fact doing the same, so we should be good here.
> 
> Yeah, that is correct/acceptable free()/grub_free() usage.
> 

Great!


I had one more comment about this patch which I forgot to send
last time:

+struct grub_json
+{
+  void *tokens;
+  char *string;
+  grub_size_t idx;
+};
+typedef struct grub_json grub_json_t;


Why is tokens declared as void* rather than jsmntok_t* ? There are a bunch of
casts back to jsmntok_t * as part of your wrapper.

Is the idea to attempt to provide a separation between grub_json_* and jsmn (to
allow other libraries to implement the JSON module?).  Or is there some sort of
pointer arithmetic I'm missing that requires the use of void*?

If it's the former, why don't we just do:

  typedef jsmntok_t grub_json_tok_t;
  struct grub_json
  {
    grub_json_tok_t *tokens;
    char *string;
    grub_size_t idx;
  };
  typedef struct grub_json grub_json_t;


For now and worry about other libraries later?


-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6043 bytes --]

On Wed, Nov 06, 2019 at 02:57:52PM +0000, Max Tottenham wrote:
> On 11/06, Daniel Kiper wrote:
> > On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote:
> > > On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> > > > On 11/05, Patrick Steinhardt wrote:
> > > > > +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?
> > >
> > > Thing is that the parser needs to modify the string. This is kind
> > > of an optimization in order to avoid the need for calling
> > > `strdup` when returning a string, e.g. in `grub_json_getstring`.
> > > We just modify the underlying string to have a '\0' in the right
> > > place and then return it directly. It might feel weird to callers
> > > that the parsed string is getting modified under their hands,
> > > which is why I decided to just `strdup` it.
> 
> Hmm I see, I suppose one final alternative would be to create a more
> difficult to use API that requires the caller to provide the memory for
> the resulting string (which would involve providing an interface to
> supply information about the length of memory required).
> 
> Something like:
> 
>     char buff[DEFAULT_LEN] = { 0 };
>     char *type;
>     grub_err_t err = grub_json_getstring(NULL, keyslot, "type", &size);
>     type = size < DEFAULT_LEN ? buff : grub_zmalloc(size);
>     grub_json_getstring(type, keyslot "type", &size);
>     ...
> 
> Although now that I mention that it seems like it's just a pain to get
> right/use, so I think we can probably just discount that one out of
> hand.
>     

Yeah, this looks a bit error prone to use. I'd say we should
agree either on using and modifying the string passed in by the
caller originally or a copy thereof. Advantage of using the
former is that the caller can decide for himself when a copy is
needed after all, which is not possible in the latter. So maybe
just avoid the copy and put some nice documentation into
"json.h"?

There are no hard feelings here on my side. For now, there's a
single user of this API, only, so we can still trivially change
this if we see the need arise.

> > > > > +  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)
> > >
> > > jsmn's examples curiously have the same realloc-loop that I do.
> > > I'll check again and definitely convert to what you propose if
> > > supported.
> > 
> 
> I think that's because they have only two examples, one that expects a
> maximum of 128 tokens and doesn't bother performing any dynamic
> allocation, and another that reads from stdin in chunks - so the
> input string length isn't known in advance.
> 
> In our case, we know the total input string size ahead of time (as it's
> embedded in the binary header).

Yeah, I skipped over the example too quick. I've changed the code
locally already and will send it out with v3 as soon as some more
reviews come in. Thanks for noticing!

> I had one more comment about this patch which I forgot to send
> last time:
> 
> +struct grub_json
> +{
> +  void *tokens;
> +  char *string;
> +  grub_size_t idx;
> +};
> +typedef struct grub_json grub_json_t;
> 
> 
> Why is tokens declared as void* rather than jsmntok_t* ? There are a bunch of
> casts back to jsmntok_t * as part of your wrapper.
> 
> Is the idea to attempt to provide a separation between grub_json_* and jsmn (to
> allow other libraries to implement the JSON module?).  Or is there some sort of
> pointer arithmetic I'm missing that requires the use of void*?
> 
> If it's the former, why don't we just do:
> 
>   typedef jsmntok_t grub_json_tok_t;
>   struct grub_json
>   {
>     grub_json_tok_t *tokens;
>     char *string;
>     grub_size_t idx;
>   };
>   typedef struct grub_json grub_json_t;
> 
> 
> For now and worry about other libraries later?

The reason is that we'd have to include "jsmn.h" in
"include/grub/json.h" to make `jsmntok_t` available. I definitely
want to avoid this in order to not leak any decls from jsmn into
users of "json.h" and to be able to have "jsmn.h" live in
"grub-core/lib/json". It's also not possible to have a forward
declaration here due to how `jsmntok_t` is declared.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Max Tottenham]

On 11/06, Patrick Steinhardt wrote:
> On Wed, Nov 06, 2019 at 02:57:52PM +0000, Max Tottenham wrote:
> > On 11/06, Daniel Kiper wrote:
> > > On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote:
> > > > On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> > > > > On 11/05, Patrick Steinhardt wrote:
> > > > > > +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?
> > > >
> > > > Thing is that the parser needs to modify the string. This is kind
> > > > of an optimization in order to avoid the need for calling
> > > > `strdup` when returning a string, e.g. in `grub_json_getstring`.
> > > > We just modify the underlying string to have a '\0' in the right
> > > > place and then return it directly. It might feel weird to callers
> > > > that the parsed string is getting modified under their hands,
> > > > which is why I decided to just `strdup` it.
> > 
> > Hmm I see, I suppose one final alternative would be to create a more
> > difficult to use API that requires the caller to provide the memory for
> > the resulting string (which would involve providing an interface to
> > supply information about the length of memory required).
> > 
> > Something like:
> > 
> >     char buff[DEFAULT_LEN] = { 0 };
> >     char *type;
> >     grub_err_t err = grub_json_getstring(NULL, keyslot, "type", &size);
> >     type = size < DEFAULT_LEN ? buff : grub_zmalloc(size);
> >     grub_json_getstring(type, keyslot "type", &size);
> >     ...
> > 
> > Although now that I mention that it seems like it's just a pain to get
> > right/use, so I think we can probably just discount that one out of
> > hand.
> >     
> 
> Yeah, this looks a bit error prone to use. I'd say we should
> agree either on using and modifying the string passed in by the
> caller originally or a copy thereof. Advantage of using the
> former is that the caller can decide for himself when a copy is
> needed after all, which is not possible in the latter. So maybe
> just avoid the copy and put some nice documentation into
> "json.h"?
> 

My preference would be the former but I'd be curious to hear what Daniel
et. al think.

> There are no hard feelings here on my side. For now, there's a
> single user of this API, only, so we can still trivially change
> this if we see the need arise.
> 
> > > > > > +  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)
> > > >
> > > > jsmn's examples curiously have the same realloc-loop that I do.
> > > > I'll check again and definitely convert to what you propose if
> > > > supported.
> > > 
> > 
> > I think that's because they have only two examples, one that expects a
> > maximum of 128 tokens and doesn't bother performing any dynamic
> > allocation, and another that reads from stdin in chunks - so the
> > input string length isn't known in advance.
> > 
> > In our case, we know the total input string size ahead of time (as it's
> > embedded in the binary header).
> 
> Yeah, I skipped over the example too quick. I've changed the code
> locally already and will send it out with v3 as soon as some more
> reviews come in. Thanks for noticing!
> 
> > I had one more comment about this patch which I forgot to send
> > last time:
> > 
> > +struct grub_json
> > +{
> > +  void *tokens;
> > +  char *string;
> > +  grub_size_t idx;
> > +};
> > +typedef struct grub_json grub_json_t;
> > 
> > 
> > Why is tokens declared as void* rather than jsmntok_t* ? There are a bunch of
> > casts back to jsmntok_t * as part of your wrapper.
> > 
> > Is the idea to attempt to provide a separation between grub_json_* and jsmn (to
> > allow other libraries to implement the JSON module?).  Or is there some sort of
> > pointer arithmetic I'm missing that requires the use of void*?
> > 
> > If it's the former, why don't we just do:
> > 
> >   typedef jsmntok_t grub_json_tok_t;
> >   struct grub_json
> >   {
> >     grub_json_tok_t *tokens;
> >     char *string;
> >     grub_size_t idx;
> >   };
> >   typedef struct grub_json grub_json_t;
> > 
> > 
> > For now and worry about other libraries later?
> 
> The reason is that we'd have to include "jsmn.h" in
> "include/grub/json.h" to make `jsmntok_t` available. I definitely
> want to avoid this in order to not leak any decls from jsmn into
> users of "json.h" and to be able to have "jsmn.h" live in
> "grub-core/lib/json". It's also not possible to have a forward
> declaration here due to how `jsmntok_t` is declared.
> 
> Patrick


In which case, looking again at this patchset - grub_json_t
is always used as an opaque value by consumers of json.h .

Why not put a forward declaration of grub_json_t in include/grub/json.h,
and stick the implementation in grub-core/lib/json.c :

  include/grub/json.h:

    typedef struct grub_json_t grub_json_t;
  
  grub-core/lib/json.c:
  ...
  #include <grub/json.h>
  #include "jsmn.h"
  ...
  struct grub_json_t {
        jsmntok_t* tokens;
        ...
  };


Unless there is something I'm missing doesn't the above achieve the
desired result? 

-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> On 11/06, Patrick Steinhardt wrote:
> > On Wed, Nov 06, 2019 at 02:57:52PM +0000, Max Tottenham wrote:
> > > I had one more comment about this patch which I forgot to send
> > > last time:
> > > 
> > > +struct grub_json
> > > +{
> > > +  void *tokens;
> > > +  char *string;
> > > +  grub_size_t idx;
> > > +};
> > > +typedef struct grub_json grub_json_t;
> > > 
> > > 
> > > Why is tokens declared as void* rather than jsmntok_t* ? There are a bunch of
> > > casts back to jsmntok_t * as part of your wrapper.
> > > 
> > > Is the idea to attempt to provide a separation between grub_json_* and jsmn (to
> > > allow other libraries to implement the JSON module?).  Or is there some sort of
> > > pointer arithmetic I'm missing that requires the use of void*?
> > > 
> > > If it's the former, why don't we just do:
> > > 
> > >   typedef jsmntok_t grub_json_tok_t;
> > >   struct grub_json
> > >   {
> > >     grub_json_tok_t *tokens;
> > >     char *string;
> > >     grub_size_t idx;
> > >   };
> > >   typedef struct grub_json grub_json_t;
> > > 
> > > 
> > > For now and worry about other libraries later?
> > 
> > The reason is that we'd have to include "jsmn.h" in
> > "include/grub/json.h" to make `jsmntok_t` available. I definitely
> > want to avoid this in order to not leak any decls from jsmn into
> > users of "json.h" and to be able to have "jsmn.h" live in
> > "grub-core/lib/json". It's also not possible to have a forward
> > declaration here due to how `jsmntok_t` is declared.
> > 
> > Patrick
> 
> 
> In which case, looking again at this patchset - grub_json_t
> is always used as an opaque value by consumers of json.h .
> 
> Why not put a forward declaration of grub_json_t in include/grub/json.h,
> and stick the implementation in grub-core/lib/json.c :
> 
>   include/grub/json.h:
> 
>     typedef struct grub_json_t grub_json_t;
>   
>   grub-core/lib/json.c:
>   ...
>   #include <grub/json.h>
>   #include "jsmn.h"
>   ...
>   struct grub_json_t {
>         jsmntok_t* tokens;
>         ...
>   };
> 
> 
> Unless there is something I'm missing doesn't the above achieve the
> desired result? 

If we'd do that, all calls to `grub_json_get{child,value}` would
have to return a dynamically allocated `grub_json_t` structure as
the storage size of `grub_json_t` would not be known at compile
time and thus cannot be allocated on the caller's stack. Right
now, the caller can just declare the structure on-stack and have
the interface fill up these structures for him, without any need
for dynamic memory allocation.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Max Tottenham]

On 11/07, Patrick Steinhardt wrote:
> On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> > On 11/06, Patrick Steinhardt wrote:
> > > 
> > > The reason is that we'd have to include "jsmn.h" in
> > > "include/grub/json.h" to make `jsmntok_t` available. I definitely
> > > want to avoid this in order to not leak any decls from jsmn into
> > > users of "json.h" and to be able to have "jsmn.h" live in
> > > "grub-core/lib/json". It's also not possible to have a forward
> > > declaration here due to how `jsmntok_t` is declared.
> > > 
> > > Patrick
> > 
> > 
> > In which case, looking again at this patchset - grub_json_t
> > is always used as an opaque value by consumers of json.h .
> > 
> > Why not put a forward declaration of grub_json_t in include/grub/json.h,
> > and stick the implementation in grub-core/lib/json.c :
> > 
> >   include/grub/json.h:
> > 
> >     typedef struct grub_json_t grub_json_t;
> >   
> >   grub-core/lib/json.c:
> >   ...
> >   #include <grub/json.h>
> >   #include "jsmn.h"
> >   ...
> >   struct grub_json_t {
> >         jsmntok_t* tokens;
> >         ...
> >   };
> > 
> > 
> > Unless there is something I'm missing doesn't the above achieve the
> > desired result? 
> 
> If we'd do that, all calls to `grub_json_get{child,value}` would
> have to return a dynamically allocated `grub_json_t` structure as
> the storage size of `grub_json_t` would not be known at compile
> time and thus cannot be allocated on the caller's stack. Right
> now, the caller can just declare the structure on-stack and have
> the interface fill up these structures for him, without any need
> for dynamic memory allocation.
> 
> Patrick

Ah, good point. I've been thinking about this for a while and I think
dealing with casts from void* really is the best we can do.

Perhaps jsmn upstream would accept a patch that does:

  - typedef struct {
  + typedef struct jsmntok_t {
    ...
    } jsmntok_t;

If they do we could revisit this later.
  
-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

On Fri, Nov 08, 2019 at 01:30:28PM +0000, Max Tottenham wrote:
> On 11/07, Patrick Steinhardt wrote:
> > On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> > > On 11/06, Patrick Steinhardt wrote:
> > > > 
> > > > The reason is that we'd have to include "jsmn.h" in
> > > > "include/grub/json.h" to make `jsmntok_t` available. I definitely
> > > > want to avoid this in order to not leak any decls from jsmn into
> > > > users of "json.h" and to be able to have "jsmn.h" live in
> > > > "grub-core/lib/json". It's also not possible to have a forward
> > > > declaration here due to how `jsmntok_t` is declared.
> > > > 
> > > > Patrick
> > > 
> > > 
> > > In which case, looking again at this patchset - grub_json_t
> > > is always used as an opaque value by consumers of json.h .
> > > 
> > > Why not put a forward declaration of grub_json_t in include/grub/json.h,
> > > and stick the implementation in grub-core/lib/json.c :
> > > 
> > >   include/grub/json.h:
> > > 
> > >     typedef struct grub_json_t grub_json_t;
> > >   
> > >   grub-core/lib/json.c:
> > >   ...
> > >   #include <grub/json.h>
> > >   #include "jsmn.h"
> > >   ...
> > >   struct grub_json_t {
> > >         jsmntok_t* tokens;
> > >         ...
> > >   };
> > > 
> > > 
> > > Unless there is something I'm missing doesn't the above achieve the
> > > desired result? 
> > 
> > If we'd do that, all calls to `grub_json_get{child,value}` would
> > have to return a dynamically allocated `grub_json_t` structure as
> > the storage size of `grub_json_t` would not be known at compile
> > time and thus cannot be allocated on the caller's stack. Right
> > now, the caller can just declare the structure on-stack and have
> > the interface fill up these structures for him, without any need
> > for dynamic memory allocation.
> > 
> > Patrick
> 
> Ah, good point. I've been thinking about this for a while and I think
> dealing with casts from void* really is the best we can do.
> 
> Perhaps jsmn upstream would accept a patch that does:
> 
>   - typedef struct {
>   + typedef struct jsmntok_t {
>     ...
>     } jsmntok_t;
> 
> If they do we could revisit this later.

I've created a pull request upstream to change exactly that --
let's wait and see. In the meantime we'll have to use the current
`void *` workaround, but I'll make sure to update this as soon as
the PR got merged (if ever).

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Kiper]

On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> On 11/06, Patrick Steinhardt wrote:
> > On Wed, Nov 06, 2019 at 02:57:52PM +0000, Max Tottenham wrote:
> > > On 11/06, Daniel Kiper wrote:
> > > > On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote:
> > > > > On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-devel wrote:
> > > > > > On 11/05, Patrick Steinhardt wrote:
> > > > > > > +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?
> > > > >
> > > > > Thing is that the parser needs to modify the string. This is kind
> > > > > of an optimization in order to avoid the need for calling
> > > > > `strdup` when returning a string, e.g. in `grub_json_getstring`.
> > > > > We just modify the underlying string to have a '\0' in the right
> > > > > place and then return it directly. It might feel weird to callers
> > > > > that the parsed string is getting modified under their hands,
> > > > > which is why I decided to just `strdup` it.
> > >
> > > Hmm I see, I suppose one final alternative would be to create a more
> > > difficult to use API that requires the caller to provide the memory for
> > > the resulting string (which would involve providing an interface to
> > > supply information about the length of memory required).
> > >
> > > Something like:
> > >
> > >     char buff[DEFAULT_LEN] = { 0 };
> > >     char *type;
> > >     grub_err_t err = grub_json_getstring(NULL, keyslot, "type", &size);
> > >     type = size < DEFAULT_LEN ? buff : grub_zmalloc(size);
> > >     grub_json_getstring(type, keyslot "type", &size);
> > >     ...
> > >
> > > Although now that I mention that it seems like it's just a pain to get
> > > right/use, so I think we can probably just discount that one out of
> > > hand.
> > >
> >
> > Yeah, this looks a bit error prone to use. I'd say we should
> > agree either on using and modifying the string passed in by the
> > caller originally or a copy thereof. Advantage of using the
> > former is that the caller can decide for himself when a copy is
> > needed after all, which is not possible in the latter. So maybe
> > just avoid the copy and put some nice documentation into
> > "json.h"?
>
> My preference would be the former but I'd be curious to hear what Daniel
> et. al think.

The former makes sense for me too. So, "just avoid the copy and put some
nice documentation into json.h" and maybe into docs/grub-dev.texi.

Daniel

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 <ps@pks.im>
> ---
>  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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

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

[Patrick Steinhardt]

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 | 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);
+  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);
+      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);
+      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