grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: [PATCH 2/6] jsmn: Add convenience functions
Date: Wed, 13 Nov 2019 12:16:17 +0100
User-agent: NeoMutt/20170113 (1.7.2)

On Wed, Nov 06, 2019 at 02:08:48PM +0100, Patrick Steinhardt wrote:
> On Wed, Nov 06, 2019 at 12:44:58PM +0100, Daniel Kiper wrote:
> > On Mon, Nov 04, 2019 at 07:56:48PM +0100, Patrick Steinhardt wrote:
> > > On Mon, Nov 04, 2019 at 06:42:51PM +0100, Daniel Kiper wrote:
> > > > 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).
> > >
> > > Thanks a lot for your advice.
> > >
> > > Just to make sure we're on the same page about the globally
> > > available stuff. I take you'd like to have "json.h" in
> > > "include/grub/json.h", but "json.h" will need to include "jsmn.h"
> > > with `#define JSMN_HEADER` in order to make struct and function
> > > declarations available. I guess it's kind of backwards to have
> > > headers in "include/" include headers from "grub-core/lib", but I
> > > can't really see a way around it without either re-declaring the
> > > same things as in "jsmn.h" or by creating a wrapping API around
> > > "jsmn.h".
> >
> > Yeah, you are right that looks strange. So, let's go zstd way and
> > put everything into grub-core/lib/json. Then add required CC/CPP
> > flags to the Makefile as zstd module does.
> >
> > Daniel
>
> Well, since writing this I've posted v2, which implements a
> complete wrapper around jsmn that looks and feels more like
> GRUB-style functions (as proposed above in my first bullet
> point). This actually does allow us to move "jsmn.h" into lib/
> and the "json.h" lives in include/grub now. So while it does add
> a few more lines compared to v1, it seems a lot cleaner to me.

I am OK with the wrapper. However, after some thinking and checking
currently existing code I would like to have all JSON related files,
including json.h, in one place, i.e. grub-core/lib/json. So, please
move json.h there too.

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]