From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1iSJqY-0008VB-Eo for mharc-grub-devel@gnu.org; Wed, 06 Nov 2019 06:51:34 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:44031) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iSJqV-0008Qw-OU for grub-devel@gnu.org; Wed, 06 Nov 2019 06:51:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iSJqU-00031G-Br for grub-devel@gnu.org; Wed, 06 Nov 2019 06:51:31 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:54746) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1iSJqU-0002ze-0j for grub-devel@gnu.org; Wed, 06 Nov 2019 06:51:30 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:52252 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S1931046AbfKFLpA (ORCPT ); Wed, 6 Nov 2019 12:45:00 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Wed, 6 Nov 2019 12:44:58 +0100 From: Daniel Kiper To: Patrick Steinhardt Cc: Max Tottenham , The development of GNU GRUB Subject: Re: [PATCH 2/6] jsmn: Add convenience functions Message-ID: <20191106114458.a3452zya7p2nm3tt@tomti.i.net-space.pl> References: <20191104102620.GA923@akamai.com> <20191104110053.GA2673@ncase> <20191104174251.u2y7dtkj4rvg5zkt@tomti.i.net-space.pl> <20191104185648.GA2540@ncase> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191104185648.GA2540@ncase> User-Agent: NeoMutt/20170113 (1.7.2) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 84.10.22.86 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Nov 2019 11:51:33 -0000 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 > > > > > --- > > > > > 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