From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1iVaDm-0004j9-D6 for mharc-grub-devel@gnu.org; Fri, 15 Nov 2019 06:57:02 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:40324) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iVaDi-0004e2-NL for grub-devel@gnu.org; Fri, 15 Nov 2019 06:56:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iVaDh-00087y-FX for grub-devel@gnu.org; Fri, 15 Nov 2019 06:56:58 -0500 Received: from dibed.net-space.pl ([84.10.22.86]:46747) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_3DES_EDE_CBC_SHA1:24) (Exim 4.71) (envelope-from ) id 1iVaDh-00087Q-4B for grub-devel@gnu.org; Fri, 15 Nov 2019 06:56:57 -0500 Received: from router-fw.i.net-space.pl ([192.168.52.1]:41284 "EHLO tomti.i.net-space.pl") by router-fw-old.i.net-space.pl with ESMTP id S1912429AbfKOL4o (ORCPT ); Fri, 15 Nov 2019 12:56:44 +0100 X-Comment: RFC 2476 MSA function at dibed.net-space.pl logged sender identity as: dkiper Date: Fri, 15 Nov 2019 12:56:40 +0100 From: Daniel Kiper To: Patrick Steinhardt Cc: grub-devel@gnu.org, Max Tottenham Subject: Re: [PATCH v3 2/6] json: Implement wrapping interface Message-ID: <20191115115640.y2ifpcoflhlkdatq@tomti.i.net-space.pl> References: <680b5add59a40fbe3dc77f8ef5eb826849fe0d37.1573651222.git.ps@pks.im> <20191114123718.bzps5ucvdqvxivcu@tomti.i.net-space.pl> <20191114131239.GA3036@ncase> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191114131239.GA3036@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: Fri, 15 Nov 2019 11:57:00 -0000 On Thu, Nov 14, 2019 at 02:12:39PM +0100, Patrick Steinhardt wrote: > 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: [...] > > > + 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; > > > > 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. OK, then leave this as is. > > > + } > > > + return GRUB_JSON_UNDEFINED; > > > +} > > > + > > > +grub_err_t > > > +grub_json_getchild(grub_json_t *out, const grub_json_t *parent, grub_size_t n) > > > +{ > > > + jsmntok_t *p = &((jsmntok_t *)parent->tokens)[parent->idx]; > > > > 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. You are not able to predict all cases. So, I am leaning toward to have checks for NULL than crashing GRUB randomly because we have not checked for it. > > > + grub_size_t offset = 1; > > > + > > > + if (n >= (unsigned) p->size) > > > > 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. If jsmn is using something "intish" then I think that we should use grub_ssize_t. Even if variables of a such type does not get negative values right now. > But you're right, I should cast to `grub_size_t` instead of > `unsigned` to make it a bit clearer here. ...grub_ssize_t then? Daniel