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

[Patrick Steinhardt]

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:
[snip]
> > +  json->string = string;
> > +  if (!json->string)
> > +    {
> > +      err = GRUB_ERR_OUT_OF_MEMORY;
> 
> Hmmm??? GRUB_ERR_BAD_ARGUMENT??? And please check string instead of
> json->string. Additionally, if you check it just at the beginning of
> the function you can do "return GRUB_ERR_BAD_ARGUMENT" immediately.

Yeah, that's a leftover of the previous call to `grub_strndup`.
Fixed and moved the check to the front.

> > +      goto out;
> > +    }
> > +
> > +  jsmn_init(&parser);
> > +  jsmn_err = jsmn_parse (&parser, string, string_len, NULL, 0);
> > +  if (jsmn_err <= 0)
> > +    {
> > +      err = GRUB_ERR_BAD_ARGUMENT;
> > +      goto out;
> > +    }
> > +
> > +  json->tokens = grub_malloc (sizeof (jsmntok_t) * jsmn_err);
> > +  if (!json->tokens)
> > +    {
> > +      err = GRUB_ERR_OUT_OF_MEMORY;
> > +      goto out;
> > +    }
> > +
> > +  jsmn_init(&parser);
> 
> Do you need to run jsmn_init() twice? By the way, missing space before "(".

Yup, indeed. jsmn allows streaming of data, which is why the
parser contains the current parsing state which needs to be reset
before doing the second pass.

[snip]
> > +  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.

> > +    }
> > +  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.

> > +  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.

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

[snip]

Thanks a lot for your review. I've addressed all of your comments
and will send them out with v4. I'll wait a few more days until I
do so in order to wait for some more reviews.

Patrick

signature.asc
Description: PGP signature