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 | mtott...@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
