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

[Daniel Kiper]

On Thu, Nov 07, 2019 at 02:51:30AM +0000, Max Tottenham via Grub-devel wrote:
> 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.

The former makes sense for me too. So, "just avoid the copy and put some
nice documentation into json.h" and maybe into docs/grub-dev.texi.

Daniel