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

[Daniel Kiper <dkiper@net-space.pl>]

On Fri, Dec 06, 2019 at 06:24:28PM +0100, Patrick Steinhardt wrote:
> On Fri, Nov 29, 2019 at 04:34:58PM +0100, Daniel Kiper wrote:
> > On Fri, Nov 29, 2019 at 07:51:46AM +0100, Patrick Steinhardt wrote:
> [snip]
> > > +grub_err_t
> > > +grub_json_getsize (grub_size_t *out, const grub_json_t *json)
> > > +{
> > > +  int size;
> >
> > I hope that ((jsmntok_t *)json->tokens)() returns int...
>
> Yeah, the JSON token's size is stored as an int.

Great!

> > > +  if (!json)
> > > +    return GRUB_ERR_BAD_ARGUMENT;
> >
> > Hmmm... I am looking at this and I have a feeling that we are not
> > consistent. If we want to be consistent we should check "out" for NULL
> > too. Same below. However, this complicates the code needlessly. How jsmn
> > copes with NULL? Does it care at all? Maybe we should just drop these
> > checks and state clearly somewhere in docs/comments that the caller must
> > provide valid pointers. Full stop!
>
> jsmn doesn't care at all, as it doesn't provide any accessing
> functions but the parsing code, only. Thus every NULL pointer
> handling needs to be done by the user.
>
> I'm fine with just saying "Give us valid pointers" as it was in
> my initial design.

Nice! ...and sorry for asking you to do it back and forth...

> > > +  *out = (size_t) size;
> >
> > s/size_t/grub_size_t/
> >
> > However, TBH I would prefer grub_ssize_t here...
>
> Instead of the error return code or in addition to that? It would
> require the caller to always check the returned size for negative
> values.

Yeah, you are right. So, please do s/size_t/grub_size_t/ only.

> > > +static grub_err_t
> > > +get_value (grub_json_type_t *out_type, const char **out_string, const grub_json_t *parent, const char *key)
> > > +{
> > > +  const grub_json_t *p = parent;
> > > +  grub_json_t child;
> > > +  grub_err_t ret;
> > > +  jsmntok_t *tok;
> > > +
> > > +  if (!parent)
> > > +    return GRUB_ERR_BAD_ARGUMENT;
> > > +
> > > +  if (key)
> > > +    {
> > > +      ret = grub_json_getvalue (&child, parent, key);
> > > +      if (ret)
> > > +	return ret;
> > > +      p = &child;
> > > +    }
> > > +
> > > +  tok = &((jsmntok_t *) p->tokens)[p->idx];
> > > +  p->string[tok->end] = '\0';
> >
> > Are you sure that tok is never NULL?
>
> Yeah, it cannot be as we're directly taking the address after
> indexing into the array. What _could_ happen is that somebody
> provided an invalid parent with a bogus index, but that would
> again only be possible by misuse of the API.

OK then. Looking forward for v6...

Daniel