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