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

[Matthew Daley <mattjd@xxxxxxxxx>]

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...)

>
> 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