[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] libxl: handle null lists in libxl_string_list_length



On 09/27/2013 09:14 AM, Matthew Daley wrote:
On Sat, Sep 28, 2013 at 12:28 AM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
On Sat, 2013-09-28 at 00:20 +1200, Matthew Daley wrote:
On Sat, Sep 28, 2013 at 12:08 AM, Boris Ostrovsky
<boris.ostrovsky@xxxxxxxxxx> wrote:
----- mattjd@xxxxxxxxx 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.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.