From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1iSXtZ-0005Kd-AQ for mharc-grub-devel@gnu.org; Wed, 06 Nov 2019 21:51:37 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:41794) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iSXtW-0005KI-PD for grub-devel@gnu.org; Wed, 06 Nov 2019 21:51:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iSXtU-0007hF-QW for grub-devel@gnu.org; Wed, 06 Nov 2019 21:51:34 -0500 Received: from [37.152.232.210] (port=50680 helo=lon-mpk47.localdomain) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1iSXtU-0007bM-GB for grub-devel@gnu.org; Wed, 06 Nov 2019 21:51:32 -0500 Received: by lon-mpk47.localdomain (Postfix, from userid 510) id 55A4F201410472; Tue, 5 Nov 2019 09:54:50 +0000 (GMT) Date: Tue, 5 Nov 2019 09:54:49 +0000 From: Max Tottenham To: The development of GNU GRUB Cc: Patrick Steinhardt Subject: Re: [PATCH v2 2/6] json: Implement wrapping interface Message-ID: <20191105095449.GA51509@lon-mpk47.lan> References: <90099e5eebbc073b82b5027d8a76d0722f79938b.1572936505.git.ps@pks.im> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <90099e5eebbc073b82b5027d8a76d0722f79938b.1572936505.git.ps@pks.im> User-Agent: Mutt/1.12.1 (2019-06-15) X-detected-operating-system: by eggs.gnu.org: Mac OS X [generic] [fuzzy] X-Received-From: 37.152.232.210 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Nov 2019 02:51:36 -0000 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 > --- > 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 > +#include > +#include > > +#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 . > + */ > + > +#ifndef GRUB_JSON_JSON_H > +#define GRUB_JSON_JSON_H 1 > + > +#include > + > +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