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

[Daniel Kiper]

On Mon, Nov 18, 2019 at 09:45:13AM +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 | 217 ++++++++++++++++++++++++++++++++++++++
>  grub-core/lib/json/json.h | 103 ++++++++++++++++++
>  2 files changed, 320 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..dd3e5da99 100644
> --- a/grub-core/lib/json/json.c
> +++ b/grub-core/lib/json/json.c
> @@ -17,7 +17,224 @@
>   */
>
>  #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_err;

s/jsmn_err/jsmn_ret/... Here and below please....

> +
> +  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_err = jsmn_parse (&parser, string, string_len, NULL, 0);
> +  if (jsmn_err <= 0)
> +    {
> +      ret = GRUB_ERR_BAD_ARGUMENT;
> +      goto err;
> +    }
> +
> +  json->tokens = grub_malloc (sizeof (jsmntok_t) * jsmn_err);
> +  if (!json->tokens)
> +    {
> +      ret = GRUB_ERR_OUT_OF_MEMORY;
> +      goto err;
> +    }
> +
> +  jsmn_init (&parser);
> +  jsmn_err = jsmn_parse (&parser, string, string_len, json->tokens, 
> jsmn_err);
> +  if (jsmn_err <= 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_size_t
> +grub_json_getsize (const grub_json_t *json)
> +{
> +  jsmntok_t *p = &((jsmntok_t *)json->tokens)[json->idx];

I still have a feeling that you should check at lease *json for NULL.
If not json->tokens and json->idx. However, then you should return
grub_ssize_t.

> +  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;
> +    }
> +
> +  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];

Ditto...

> +  grub_size_t offset = 1;
> +
> +  if (n >= (grub_size_t) p->size)
> +    return GRUB_ERR_BAD_ARGUMENT;
> +
> +  while (n--)
> +    n += p[offset++].size;

This looks strange. You once decrease and then increase n. Could you put
a comment what happens here?

> +  out->string = parent->string;
> +  out->tokens = parent->tokens;
> +  out->idx = parent->idx + offset;
> +
> +  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..d38e4a407
> --- /dev/null
> +++ b/grub-core/lib/json/json.h
> @@ -0,0 +1,103 @@
> +/*
> + *  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;

All member names in one column please...

Daniel