[dpdk-dev] [PATCH] cmdline: fix unchecked return value
Olivier MATZ
olivier.matz at 6wind.com
Fri Jul 1 09:19:01 CEST 2016
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
More information about the dev
mailing list