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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]