Re: [dpdk-dev] [PATCH] cmdline: fix unchecked return value

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Daniel,

>>> --- a/lib/librte_cmdline/cmdline_rdline.c
>>> +++ b/lib/librte_cmdline/cmdline_rdline.c
>>> @@ -377,7 +377,10 @@ rdline_char_in(struct rdline *rdl, char c)
>>>  		case CMDLINE_KEY_CTRL_K:
>>>  			cirbuf_get_buf_head(&rdl->right, rdl->kill_buf,
>> RDLINE_BUF_SIZE);
>>>  			rdl->kill_size = CIRBUF_GET_LEN(&rdl->right);
>>> -			cirbuf_del_buf_head(&rdl->right, rdl->kill_size);
>>> +
>>> +			if (cirbuf_del_buf_head(&rdl->right, rdl->kill_size) < 0)
>>> +					return -EINVAL;
>>> +
>>>  			rdline_puts(rdl, vt100_clear_right);
>>>  			break;
>>>
>>
>> I wonder if a better way to fix wouldn't be to remove the checks
>> introduced in http://dpdk.org/browse/dpdk/commit/?id=ab971e562860
>>
>> There is no reason to check that in cirbuf_get_buf_head/tail():
>>    if (!cbuf || !c)
>>
>> The function should never fail, it just returns the number of
>> copied chars. This is the responsibility of the caller to ensure
>> that the pointer to the circular buffer is not NULL.
>>
>> Also, rdline_char_in() is not expected to return -EINVAL, but
>> RDLINE_RES_* instead.
>>
>> So I think that partially revert ab971e562860 would fix the
>> coverity warning.
>>
>> Regards,
>> Olivier
> 
> Removing checks probably will generate more Coverity errors somewhere.
> I see that only places where we test negative values are in unit tests.
> 
> Reverting changes I think is overhead and maybe ignoring this patch and set is as false positive in Coverity is better idea ?

We can mark the warning as false positive because this cannot happen
right now (the calller checks the validity of cbuf/c).

But this is probably something I'll come back on with a patch since
there is no reason to check that pointers are not NULL in
cirbuf_get_buf_head/tail().

Regards,
Olivier