Re: [PATCH] libxl: handle null lists in libxl_string_list_length

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 09/27/2013 09:14 AM, Matthew Daley wrote:
> On Sat, Sep 28, 2013 at 12:28 AM, Ian Campbell <Ian.Campbell@citrix.com> 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@oracle.com> 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.
>>