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

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

On Thu, Nov 14, 2019 at 02:12:39PM +0100, Patrick Steinhardt wrote:
> On Thu, Nov 14, 2019 at 01:37:18PM +0100, Daniel Kiper wrote:
> > On Wed, Nov 13, 2019 at 02:22:34PM +0100, Patrick Steinhardt wrote:

[...]

> > > +  switch (p->type)
> > > +    {
> > > +    case JSMN_OBJECT:
> > > +      return GRUB_JSON_OBJECT;
> > > +    case JSMN_ARRAY:
> > > +      return GRUB_JSON_ARRAY;
> > > +    case JSMN_STRING:
> > > +      return GRUB_JSON_STRING;
> > > +    case JSMN_PRIMITIVE:
> > > +      return GRUB_JSON_PRIMITIVE;
> > > +    default:
> > > +      break;
> >
> > You do not need break here.
>
> The compiler complains if I don't though. Same for just leaving
> out the default case as GCC will complain that certain enum
> values aren't handled at all.

OK, then leave this as is.

> > > +    }
> > > +  return GRUB_JSON_UNDEFINED;
> > > +}
> > > +
> > > +grub_err_t
> > > +grub_json_getchild(grub_json_t *out, const grub_json_t *parent, grub_size_t n)
> > > +{
> > > +  jsmntok_t *p = &((jsmntok_t *)parent->tokens)[parent->idx];
> >
> > Should not you check parent for NULL first? Same thing for functions
> > above and below.
>
> I'm not too sure about this. Calling with a NULL pointer wouldn't
> make any sense whatsoever, as you cannot get anything from
> nothing. It would certainly be the more defensive approach to
> check for NULL here, and if that's GRUB's coding style then I'm
> fine with that. If not, I'd say we should just crash with NPE to
> detect misuse of the API early on.

You are not able to predict all cases. So, I am leaning toward to have
checks for NULL than crashing GRUB randomly because we have not checked
for it.

> > > +  grub_size_t offset = 1;
> > > +
> > > +  if (n >= (unsigned) p->size)
> >
> > Should not you cast to grub_size_t? Or should n be type of p->size?
> > Then you would avoid a cast.
>
> I find the choice of `int` quite weird on jsmn's side: there's
> not a single place where the size field is getting a negative
> assignment. So if you ask me, `size_t` or even just `unsigned
> int` would have been the better choice here, which is why I just
> opted for `grub_size_t` instead in our wrapping interface.

If jsmn is using something "intish" then I think that we should use
grub_ssize_t. Even if variables of a such type does not get negative
values right now.

> But you're right, I should cast to `grub_size_t` instead of
> `unsigned` to make it a bit clearer here.

...grub_ssize_t then?

Daniel