From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH] libxl: handle null lists in libxl_string_list_length Date: Fri, 27 Sep 2013 09:28:17 -0400 Message-ID: <524587F1.6050406@oracle.com> References: <1380284908.29483.188.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Matthew Daley Cc: Andrew Cooper , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 09/27/2013 09:14 AM, Matthew Daley wrote: > On Sat, Sep 28, 2013 at 12:28 AM, Ian Campbell wrote: >> On Sat, 2013-09-28 at 00:20 +1200, Matthew Daley wrote: >>> On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky >>> wrote: >>>> ----- mattjd@gmail.com wrote: >>>> >>>>> After commit b0be2b12 ("libxl: fix libxl_string_list_length and its >>>>> only >>>>> caller") libxl_string_list_length no longer handles null (empty) >>>>> lists. Fix >>>>> so they are handled, returning length 0. >>>>> >>>>> While at it, remove the unneccessary undereferenced null pointer >>>>> check >>>> Are you sure this check should be removed? This routine can be called >>>> from anywhere (at least within libxl it seems) and one day someone will >>>> call it with NULL argument. >>>> >>>> I'd probably leave this check in. >>> I would argue that any such invocation would be an error by the caller >>> and should fail noisily, similar to how passing NULL into strlen >>> should not return 0. libxl_{string,key_value}_list_dispose similarly >>> assumes non-NULL pointers, FWIW. >>> >>> Ian C., do you have an opinion either way? >> I think a zero length list is a bit different to a NULL string and >> should return 0. > Perhaps it was a bad analogy, but passing NULL to this function isn't > giving it an empty list, it's giving it no (NULL!) list. We don't > check for null pointers everywhere else non-optional pointers are > passed (at least, we shouldn't be, IMO...) What if someone assigns 'libxl_string_list *psl = NULL' if, say, main()'s argc is 1 (i.e. there is no argument list) and then, later, calls libxl_string_list_length(psl) to find out whether something needs to be allocated for the list. Isn't getting a zero back an expected answer? (I am afraid we are approaching rathole territory here.) -boris > >> But libxl_string_list is already char** so this function is taking >> char***. The check for char *** == NULL, which is being removed, appears >> to be unnecessary. A zero length list would be char ** == NULL, which >> should be handled (and is I think?). char * == NULL would be a "" entry >> in the string list. > This was my intention in this patch; only the char *** == NULL check > is removed, and the char ** == NULL for empty lists is handled by the > newly added if condition. > > But char * == NULL doesn't mean an "" entry, doesn't it instead mark > the end of the list (see xlu_cfg_get_list_as_string_list for example)? > This is currently being checked for in the while loop condition. To > continue using your notation, instead char == NULL is an empty string > value in the list. > >> Confused? I know I am ;-) > :) > > - Matthew > >> Ian. >>