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

["Mrzyglod, DanielX T" <danielx.t.mrzyglod@intel.com>]

>From: Olivier Matz [mailto:olivier.matz@6wind.com]
>Sent: Monday, May 02, 2016 3:37 PM
>To: Mrzyglod, DanielX T <danielx.t.mrzyglod@intel.com>; dev@dpdk.org
>Subject: Re: [PATCH] cmdline: fix unchecked return value
>
>Hi Daniel,
>
>On 04/14/2016 03:01 PM, Daniel Mrzyglod wrote:
>> This patch is for checking if error values occurs.
>> fix for coverity errors #13209 & #13195
>>
>> If the function returns an error value, the error value may be mistaken
>> for a normal value.
>>
>> In rdline_char_in: Value returned from a function is not checked for errors
>> before being used
>>
>> Signed-off-by: Daniel Mrzyglod <danielx.t.mrzyglod@intel.com>
>> ---
>>  lib/librte_cmdline/cmdline_rdline.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_cmdline/cmdline_rdline.c
>b/lib/librte_cmdline/cmdline_rdline.c
>> index 1ef2258..e75a556 100644
>> --- 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 ?

Regards
Daniel