From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1iSRnf-0001re-HX for mharc-grub-devel@gnu.org; Wed, 06 Nov 2019 15:21:07 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:45873) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iSRnb-0001qW-SD for grub-devel@gnu.org; Wed, 06 Nov 2019 15:21:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iSRna-0006gU-8q for grub-devel@gnu.org; Wed, 06 Nov 2019 15:21:03 -0500 Received: from wout2-smtp.messagingengine.com ([64.147.123.25]:35131) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iSRnY-0006eP-Ks for grub-devel@gnu.org; Wed, 06 Nov 2019 15:21:01 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id EC0F34E8; Wed, 6 Nov 2019 15:20:56 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Wed, 06 Nov 2019 15:20:57 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=FKCO/0tLjxjWG8umJH2f77xMw6c s0pncdmwzoh50h4g=; b=zlnlG3IkwBX3jEL6FU5jjt7HTwN05GO7QFKI2ypGbBi UOWB24tKXHtcRc5F+Wcy4Y3uq1favanTO472AeD3Y0lM/08rfN9Ts0Gk6pU+66ST z7R8iFi0XIzNQ1nXa+H6B/5vM0qlz53z/s94PVI61PZ8daWlv04VcliUlZpKx07l NLj5BhFC5dpIGIVan9Ick6NhcJdQpE2igOrfTdICnSQbXDiGGpvQakX02jzW+P4n 6HVKUvjorRcxio6wcHpripp/IPjbAns7EjSUWHuy4q9N5Q61QazNMFN5XNEWpreA xaeehb97jZsB2tOUhPPFvEB1cnEprKSZUQvG8H54o5g== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=FKCO/0 tLjxjWG8umJH2f77xMw6cs0pncdmwzoh50h4g=; b=uBExG6QHT5WxjoDxEx8vRt Y02alo+j6b2uEBy7zwJYxPTH5NlG18aQVb+9vnXJTwuke5Bl3ros/DtFTf0VcYsk JLnvxy45emxuI7bp8dUSMH6mVgTWG5/XsjHCWnPR8KSrxTgztmj86BJj/wzkYRxl uOBEAq0mE/Lzr5Uej0IZ01FuQhrYPBVtkTdChW1URxsb6Uk6Pg2A5Ec19LAa0QXZ dU+uN6rD909CBGKeTOHRuKUflA+CNvYOs6mVRA7qSxAzZkf9dd7bPIY4uP1uhCBl vsRn2u6IUiZ5/XFzhFGswTfsRBdfIS+v/RwWNEyaHLu+J/Y0yanW+gb5h6XGxmTQ == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedruddujedgudefkecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpeffhffvuffkfhggtggujgesghdtreertddtvdenucfhrhhomheprfgrthhr ihgtkhcuufhtvghinhhhrghrughtuceophhssehpkhhsrdhimheqnecukfhppeekledrud dvrdduudehrdduieenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimhen ucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from NSJAIL (x590c7310.dyn.telefonica.de [89.12.115.16]) by mail.messagingengine.com (Postfix) with ESMTPA id 691818005C; Wed, 6 Nov 2019 15:20:55 -0500 (EST) Received: from localhost (10.192.0.11 [10.192.0.11]) by NSJAIL (OpenSMTPD) with ESMTPSA id 24a35b75 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 6 Nov 2019 20:20:52 +0000 (UTC) Date: Wed, 6 Nov 2019 21:20:52 +0100 From: Patrick Steinhardt To: Max Tottenham Cc: Daniel Kiper , The development of GNU GRUB Subject: Re: [PATCH v2 2/6] json: Implement wrapping interface Message-ID: <20191106202052.GA93095@ncase> References: <20191105125438.GA20442@lon-lp55b.london.corp.akamai.com> <20191105131213.GA177916@ncase> <20191106115319.wx2qowxk22xo4zot@tomti.i.net-space.pl> <20191106145752.GE20442@lon-lp55b.london.corp.akamai.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lrZ03NoBR/3+SXJZ" Content-Disposition: inline In-Reply-To: <20191106145752.GE20442@lon-lp55b.london.corp.akamai.com> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 64.147.123.25 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 20:21:05 -0000 --lrZ03NoBR/3+SXJZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 06, 2019 at 02:57:52PM +0000, Max Tottenham wrote: > On 11/06, Daniel Kiper wrote: > > On Tue, Nov 05, 2019 at 02:12:13PM +0100, Patrick Steinhardt wrote: > > > On Tue, Nov 05, 2019 at 12:54:38PM +0000, Max Tottenham via Grub-deve= l wrote: > > > > On 11/05, Patrick Steinhardt wrote: > > > > > +grub_err_t > > > > > +grub_json_parse (grub_json_t **out, const char *string, grub_siz= e_t string_len) > > > > > +{ > > > > > + grub_size_t ntokens =3D 128; > > > > > + grub_json_t *json =3D NULL; > > > > > + jsmn_parser parser; > > > > > + grub_err_t err; > > > > > + int jsmn_err; > > > > > + > > > > > + json =3D grub_zalloc (sizeof (*json)); > > > > > + if (!json) > > > > > + return GRUB_ERR_OUT_OF_MEMORY; > > > > > + json->idx =3D 0; > > > > > + json->string =3D grub_strndup (string, string_len); > > > > > > > > I'm assuming the idea here is to ensure that the lifetime of the > > > > returned grub_json_t doesn't depend on the lifetime of the input st= ring? > > > > > > > > This concerns me a little bit, from a quick scan - given your usage= of > > > > grub_json_parse in the luks2 module it appears that json_header (the > > > > underlying input string) will always outlive the json object. > > > > > > > > In which case, as there are no other existing users, we may want to= make > > > > the API contract "The lifetime/validity of the returned object is b= ound > > > > by that of the input string", if we can comment/document that clear= ly > > > > then there is no need for this extra allocation and we can simply d= o: > > > > > > > > json->string =3D string; > > > > > > > > > > > > Thoughts? > > > > > > Thing is that the parser needs to modify the string. This is kind > > > of an optimization in order to avoid the need for calling > > > `strdup` when returning a string, e.g. in `grub_json_getstring`. > > > We just modify the underlying string to have a '\0' in the right > > > place and then return it directly. It might feel weird to callers > > > that the parsed string is getting modified under their hands, > > > which is why I decided to just `strdup` it. >=20 > Hmm I see, I suppose one final alternative would be to create a more > difficult to use API that requires the caller to provide the memory for > the resulting string (which would involve providing an interface to > supply information about the length of memory required). >=20 > Something like: >=20 > char buff[DEFAULT_LEN] =3D { 0 }; > char *type; > grub_err_t err =3D grub_json_getstring(NULL, keyslot, "type", &size); > type =3D size < DEFAULT_LEN ? buff : grub_zmalloc(size); > grub_json_getstring(type, keyslot "type", &size); > ... >=20 > Although now that I mention that it seems like it's just a pain to get > right/use, so I think we can probably just discount that one out of > hand. > =20 Yeah, this looks a bit error prone to use. I'd say we should agree either on using and modifying the string passed in by the caller originally or a copy thereof. Advantage of using the former is that the caller can decide for himself when a copy is needed after all, which is not possible in the latter. So maybe just avoid the copy and put some nice documentation into "json.h"? There are no hard feelings here on my side. For now, there's a single user of this API, only, so we can still trivially change this if we see the need arise. > > > > > + if (!json->string) > > > > > + { > > > > > + err =3D GRUB_ERR_OUT_OF_MEMORY; > > > > > + goto out; > > > > > + } > > > > > + > > > > > + jsmn_init(&parser); > > > > > + > > > > > + while (1) > > > > > + { > > > > > + json->tokens =3D grub_realloc (json->tokens, sizeof (jsmnt= ok_t) * ntokens); > > > > > > > > According to the docs, calling jsmn_parse with tokens =3D NULL will= return > > > > the number of tokens required to properly parse the JSON, without t= rying > > > > to 'allocate' them in the token buffer (with the 'ntokens' parameter > > > > being ignored) > > > > > > jsmn's examples curiously have the same realloc-loop that I do. > > > I'll check again and definitely convert to what you propose if > > > supported. > >=20 >=20 > I think that's because they have only two examples, one that expects a > maximum of 128 tokens and doesn't bother performing any dynamic > allocation, and another that reads from stdin in chunks - so the > input string length isn't known in advance. >=20 > In our case, we know the total input string size ahead of time (as it's > embedded in the binary header). Yeah, I skipped over the example too quick. I've changed the code locally already and will send it out with v3 as soon as some more reviews come in. Thanks for noticing! > I had one more comment about this patch which I forgot to send > last time: >=20 > +struct grub_json > +{ > + void *tokens; > + char *string; > + grub_size_t idx; > +}; > +typedef struct grub_json grub_json_t; >=20 >=20 > Why is tokens declared as void* rather than jsmntok_t* ? There are a bunc= h of > casts back to jsmntok_t * as part of your wrapper. >=20 > Is the idea to attempt to provide a separation between grub_json_* and js= mn (to > allow other libraries to implement the JSON module?). Or is there some s= ort of > pointer arithmetic I'm missing that requires the use of void*? >=20 > If it's the former, why don't we just do: >=20 > typedef jsmntok_t grub_json_tok_t; > struct grub_json > { > grub_json_tok_t *tokens; > char *string; > grub_size_t idx; > }; > typedef struct grub_json grub_json_t; >=20 >=20 > For now and worry about other libraries later? The reason is that we'd have to include "jsmn.h" in "include/grub/json.h" to make `jsmntok_t` available. I definitely want to avoid this in order to not leak any decls from jsmn into users of "json.h" and to be able to have "jsmn.h" live in "grub-core/lib/json". It's also not possible to have a forward declaration here due to how `jsmntok_t` is declared. Patrick --lrZ03NoBR/3+SXJZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl3DKyEACgkQEXxntp6r 8SzelA/9GyXHXY0BymILSp2r7c4xjJk680pXL8/RCGMTUeiKVKAnEL7MFK4DnYcM Kom0DWp/8Rr7lwQbPA/yTax6SADafZ3k30CfTmE05mLXcQA8SZQy6EuIBKY0sepY 6qLVm9+W+6e6tQQzluF4Jr0hEfyEX5TeK2wLqoccSHrmSCjdEjqx6eZMdqrpEthU F3bEoVoo3RpU5R0TF2e7yXo0cB7F0sg4ESSC9nGFLXhVjCJHwehbTYTZLTUkuPAZ kdyfAv7U2AGuRQVAu/fnVjBbck7LN8Ewa2RMloEWvd2kNgLia0V2IIU79+CsP+LL gusezwX0W0gPWPj6kPXRHHzNjz5uN1XYCfHmxR1XhEFUtBhHkI5rRq33VAX0VCg+ 3vTGfRGx/n/rsjO6Vhz5btjVK37EkyEMZxxf0NjtAlmjK47vxep6tEPc9B3GAcvx PQ8F+3T3vhsZvT21p79hEBGNsPbk9DdmUrpT5Jeh522jh/X/CjWGVQkayTmf46W1 P4XfVPpozmHMCct2RhL0dOK/2t1dROt9OzooFR1LOjYI9nvnkjHje7ZRkxiTRcRg SfAZnV5zQ84i92P3VCjYW6kMvlgIYsxurkdjdm4ffeK08+Cmlp9HYZ6EbI/qYiAA V/J2b/7k+0DqsaEOq9hFSirwc9KrwP0feast3+Nj95SWTP+aKUo= =VRHC -----END PGP SIGNATURE----- --lrZ03NoBR/3+SXJZ--