From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1iVEvY-0000CZ-Dx for mharc-grub-devel@gnu.org; Thu, 14 Nov 2019 08:12:48 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:58154) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iVEvV-0000Aq-M6 for grub-devel@gnu.org; Thu, 14 Nov 2019 08:12:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iVEvU-0002Bc-BA for grub-devel@gnu.org; Thu, 14 Nov 2019 08:12:45 -0500 Received: from wout2-smtp.messagingengine.com ([64.147.123.25]:43423) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iVEvT-0002Ao-O4 for grub-devel@gnu.org; Thu, 14 Nov 2019 08:12:44 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id E98B337A; Thu, 14 Nov 2019 08:12:41 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Thu, 14 Nov 2019 08:12:42 -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=QLTrcxB/l35E5kV1xYvF7zkSUf5 19c7XWYAEn4zpv74=; b=j/PXmlxQ6IRLnWkjE0RshEV6fKtcIhcpo3PlFyY3oPc nOaSe4V58uvrTlmQp7yPL1LDc8JYNI4AB/elyOf+z9otMCuoKF7EYKbv/FuM4gHR 0f6hAR0ZDxO+TSbZNeHQM93k5FQBlUkI2G4LhlhSyenWm/x9JY6YVGPKnLDy7/pP LwDe/CAPX44jTXkEm6ypPx4jg/MN2yCqo2iuYXJhfW0n1eUVy8mILWpgQl1Cx/vn My62tCD+oY4+stjyu7c4NSDCoxx4KBxl2/rGO3ju6MpEt3309VDdc48Tgt2YbS1A 58Nou5XVL1CLw1Vw9jm682xgdNLpvqF1ThDfQJhvcoA== 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=QLTrcx B/l35E5kV1xYvF7zkSUf519c7XWYAEn4zpv74=; b=HvUEFcArlWwUKIwjVjkthf OElSB7Tjf5U886LMBD07IDjWdUAqCcdRyl6YfXHjzTQ8vQT9xgRU2JWvjUWrpebe pMtNDw4hnQFvTzNc8RneHCgBTHRyiDdMfn0alZutgMuH+DvNEDGUUCv1jc4W+q/R qRRNmH7aJpgfcKTSwK7xUS4vHBnZN1NNXeQjSq6yke6vEu4Zp52r+rmdCvfm9D6N scfRX0nn+Wj8XhMSnS+6DGzpoxOlkbyBSVt78XAEianFou9XiAt70KVWsjga91Rm Qnlzm0dPrAPHAq+QhsFFpjiUnF9scRl4fBKzhpODS5lVnQC3ZOaY2cfw+Z5wg6vw == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedufedrudeffedghedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucfkphepjeejrddufe drudelgedruddufeenucfrrghrrghmpehmrghilhhfrhhomhepphhssehpkhhsrdhimhen ucevlhhushhtvghrufhiiigvpedt X-ME-Proxy: Received: from NSJAIL (x4d0dc271.dyn.telefonica.de [77.13.194.113]) by mail.messagingengine.com (Postfix) with ESMTPA id 873FE3060061; Thu, 14 Nov 2019 08:12:40 -0500 (EST) Received: from localhost ( [10.192.0.11]) by NSJAIL (OpenSMTPD) with ESMTPSA id 26af8937 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Thu, 14 Nov 2019 13:12:37 +0000 (UTC) Date: Thu, 14 Nov 2019 14:12:39 +0100 From: Patrick Steinhardt To: Daniel Kiper Cc: grub-devel@gnu.org, Max Tottenham Subject: Re: [PATCH v3 2/6] json: Implement wrapping interface Message-ID: <20191114131239.GA3036@ncase> References: <680b5add59a40fbe3dc77f8ef5eb826849fe0d37.1573651222.git.ps@pks.im> <20191114123718.bzps5ucvdqvxivcu@tomti.i.net-space.pl> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="r5Pyd7+fXNt84Ff3" Content-Disposition: inline In-Reply-To: <20191114123718.bzps5ucvdqvxivcu@tomti.i.net-space.pl> 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: Thu, 14 Nov 2019 13:12:47 -0000 --r5Pyd7+fXNt84Ff3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: [snip] > > + json->string =3D string; > > + if (!json->string) > > + { > > + err =3D GRUB_ERR_OUT_OF_MEMORY; >=20 > Hmmm??? GRUB_ERR_BAD_ARGUMENT??? And please check string instead of > json->string. Additionally, if you check it just at the beginning of > the function you can do "return GRUB_ERR_BAD_ARGUMENT" immediately. Yeah, that's a leftover of the previous call to `grub_strndup`. Fixed and moved the check to the front. > > + goto out; > > + } > > + > > + jsmn_init(&parser); > > + jsmn_err =3D jsmn_parse (&parser, string, string_len, NULL, 0); > > + if (jsmn_err <=3D 0) > > + { > > + err =3D GRUB_ERR_BAD_ARGUMENT; > > + goto out; > > + } > > + > > + json->tokens =3D grub_malloc (sizeof (jsmntok_t) * jsmn_err); > > + if (!json->tokens) > > + { > > + err =3D GRUB_ERR_OUT_OF_MEMORY; > > + goto out; > > + } > > + > > + jsmn_init(&parser); >=20 > Do you need to run jsmn_init() twice? By the way, missing space before "(= ". Yup, indeed. jsmn allows streaming of data, which is why the parser contains the current parsing state which needs to be reset before doing the second pass. [snip] > > + 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; >=20 > 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. > > + } > > + return GRUB_JSON_UNDEFINED; > > +} > > + > > +grub_err_t > > +grub_json_getchild(grub_json_t *out, const grub_json_t *parent, grub_s= ize_t n) > > +{ > > + jsmntok_t *p =3D &((jsmntok_t *)parent->tokens)[parent->idx]; >=20 > 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. > > + grub_size_t offset =3D 1; > > + > > + if (n >=3D (unsigned) p->size) >=20 > 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. But you're right, I should cast to `grub_size_t` instead of `unsigned` to make it a bit clearer here. [snip] Thanks a lot for your review. I've addressed all of your comments and will send them out with v4. I'll wait a few more days until I do so in order to wait for some more reviews. Patrick --r5Pyd7+fXNt84Ff3 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEtmscHsieVjl9VyNUEXxntp6r8SwFAl3NUsUACgkQEXxntp6r 8SziqxAAoyUuNkypcWKURFuZU0wGzmpqsxpxjAQ7kzyisxIk/vYCDYDmuZETiUTY J08ILvzkfaQigqwLoWTfkXWCtLRf1dGS8mdmEeOyqYF2NnPq9OHrB8H3S7VKpold GP0k8JuVDGxx7UrmspuR0ZJZk98XcpexQ50etFwcUMYdM7gKcDJ2AC5Y2LRSAWqE 7UI9hCbTluRO1AWevk1Laun9xSXeNmNyFMiWsBi7iMEEzWAyP7bMkHOhLRs6u8OE OFRb2UcAuTozhM5mKmLgt0YiIEUV7zQ4K3tGHHeawhaJu1WqFA5kLRWZ/4RZWcsL hWTKiOIqW1kuNOj6lWB0OVwX9+rubjA7CdUInc2MN9yUMRbhU7eOeEB+mr+2XLtr hHMNpH9Aysdsdme6PMqM2rwAYOdx6nTTtDZlJyUN/QiL7UPjul6S51bqMXgOHYaS Cptli4iZqCrfzvQd3SiV6B1niqXBSlUc2Avjc508JM6oejdEsEensw8q0q7KegTf 7QP3C5Y8ccnpKnf/cYko4ZNIO3eZQsbKHTJHjds3wDFZZHMwxo+sSObpW7FKDoj7 EmChSfxWY3sY5Sc+qVteVsIwpv73+WEgJCKb6IZ+ZIzjIqDdQqA6gbF1Ig2Y92xP NA1Pee8NKAVIBHFZMR5uYVoVtMItOW5mQdp7t27BI13FPzO4VTs= =aIMi -----END PGP SIGNATURE----- --r5Pyd7+fXNt84Ff3--