Re: [PATCH 2/6] jsmn: Add convenience functions

[Daniel Kiper]

On Mon, Nov 04, 2019 at 12:00:53PM +0100, Patrick Steinhardt wrote:
> On Mon, Nov 04, 2019 at 10:26:21AM +0000, Max Tottenham wrote:
> > On 11/02, Patrick Steinhardt wrote:
> > > The newly added jsmn library is a really bare-bones library that
> > > focusses on simplicity. Because of that, it is lacking some functions
> > > for convenience to abstract away some of its inner workings and to make
> > > code easier to read. As such, we're now adding some functions that are
> > > going to be used by the LUKS2 implementation later on.
> > >
> > > Signed-off-by: Patrick Steinhardt <address@hidden>
> > > ---
> > >  include/grub/jsmn.h | 108 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 108 insertions(+)
> > >
> >
> > Would it not make sense to keep the additions in a separate header from
> > the vendored upstream library? That way it'll likely be easier to pull
> > in any updates.
>
> Yeah, I thought about that, too. I wasn't sure about where
> "jsmn.h" and our own extension should live, though, which is why
> I just bailed for now and waited on some feedback. I could
> imagine two things:
>
>     - We create "include/grub/json.h" with an API that uses
>       GRUB's coding style. It would thus work as a wrapper around
>       the jsmn API and contain functions like "grub_json_parse",
>       "grub_json_get_object" and also a struct "grub_json_t" that
>       contains all things required to work with the parsed JSON
>       object. The implementation would be contained in
>       "gurb-core/lib/json.c" and use "grub-core/lib/jsmn.h",
>       which would be the unmodified upstream library. This would
>       allow us to swap out the JSON parser in the future more
>       easily without breaking any users and also make the API
>       feel less foreign.
>
>     - Or there is both a "include/grub/jsmn.h" and
>       "include/grub/json.h", where the former one is the
>       unmodified JSMN dependency and the latter one provides our
>       own extended functions.

I would like to see JSON functionality in a module. Good example is
in commit 461f1d8af (zstd: Import upstream zstd-1.3.6). So, please
create grub-core/lib/json and put jsmn.h there in unmodified form.
Then create json.c and put all required module and convenience
functions there. If you need some globally available stuff please
put it into include/grub/json.h. Last but not least, I want to ask
you to describe jsmn.h import steps in docs/grub-dev.texi. Good example
is in commit 35b909062 (gnulib: Upgrade Gnulib and switch to bootstrap
tool).

I will send you more comments in the following days...

Daniel