[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/6] json: Implement wrapping interface
From: |
Daniel Kiper |
Subject: |
Re: [PATCH v3 2/6] json: Implement wrapping interface |
Date: |
Fri, 15 Nov 2019 12:56:40 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
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
- Re: [PATCH v2 2/6] json: Implement wrapping interface, (continued)
- [PATCH v3 0/6] Support for LUKS2 disk encryption, Patrick Steinhardt, 2019/11/13
- [PATCH v3 3/6] bootstrap: Add gnulib's base64 module, Patrick Steinhardt, 2019/11/13
- [PATCH v3 2/6] json: Implement wrapping interface, Patrick Steinhardt, 2019/11/13
- Re: [PATCH v3 2/6] json: Implement wrapping interface, Daniel Kiper, 2019/11/14
- Re: [PATCH v3 2/6] json: Implement wrapping interface, Patrick Steinhardt, 2019/11/14
- Re: [PATCH v3 2/6] json: Implement wrapping interface,
Daniel Kiper <=
- Re: [PATCH v3 2/6] json: Implement wrapping interface, Patrick Steinhardt, 2019/11/15
- Re: [PATCH v3 2/6] json: Implement wrapping interface, Daniel Kiper, 2019/11/18
- Re: [PATCH v3 2/6] json: Implement wrapping interface, Patrick Steinhardt, 2019/11/26
[PATCH v3 4/6] afsplitter: Move into its own module, Patrick Steinhardt, 2019/11/13
[PATCH v3 1/6] json: Import upstream jsmn-1.1.0, Patrick Steinhardt, 2019/11/13
[PATCH v3 5/6] luks: Move configuration of ciphers into cryptodisk, Patrick Steinhardt, 2019/11/13
[PATCH v3 6/6] disk: Implement support for LUKS2, Patrick Steinhardt, 2019/11/13