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       | address@hidden
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies