>-----Original Message-----
>From: Richardson, Bruce
>Sent: Tuesday, May 3, 2016 4:34 PM
>To: Mrozowicz, SlawomirX <slawomirx.mrozowicz at intel.com>
>Cc: dev at dpdk.org
>Subject: Re: [PATCH] lpm: unchecked return value
>
>On Wed, Apr 27, 2016 at 02:52:34PM +0200, Slawomir Mrozowicz wrote:
>> Fix issue reported by Coverity.
>>
>> Coverity ID 13205: Unchecked return value Unchecked return value
>> check_return: Calling rte_lpm6_add without checking return value
>> Fixes: 5c510e13a9cb ("lpm: add IPv6 support")
>>
>> Signed-off-by: Slawomir Mrozowicz <slawomirx.mrozowicz at intel.com>
>> ---
>> lib/librte_lpm/rte_lpm6.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
>> index ba4353c..f4db3fa 100644
>> --- a/lib/librte_lpm/rte_lpm6.c
>> +++ b/lib/librte_lpm/rte_lpm6.c
>> @@ -749,6 +749,7 @@ rte_lpm6_delete(struct rte_lpm6 *lpm, uint8_t *ip,
>uint8_t depth)
>> int32_t rule_to_delete_index;
>> uint8_t ip_masked[RTE_LPM6_IPV6_ADDR_SIZE];
>> unsigned i;
>> + int status = 0;
>>
>> /*
>> * Check input arguments.
>> @@ -790,12 +791,13 @@ rte_lpm6_delete(struct rte_lpm6 *lpm, uint8_t
>*ip, uint8_t depth)
>> * Add every rule again (except for the one that was removed from
>> * the rules table).
>> */
>> - for (i = 0; i < lpm->used_rules; i++) {
>> - rte_lpm6_add(lpm, lpm->rules_tbl[i].ip, lpm-
>>rules_tbl[i].depth,
>> - lpm->rules_tbl[i].next_hop);
>> + for (i = 0; i < lpm->used_rules && status >= 0; i++) {
>> + status = rte_lpm6_add(
>> + lpm, lpm->rules_tbl[i].ip, lpm->rules_tbl[i].depth,
>> + lpm->rules_tbl[i].next_hop);
>> }
>>
>> - return 0;
>> + return status;
>> }
>
>Hi,
>
>I'm not sure that this patch is actually necessary, as I'm not sure that the
>lpm6_add calls can fail in this instance. Looking through the code, this
>function
>deletes the rule and then clears the actual lpm lookup tables before re-adding
>all other routes to it again. The only error condition that could be returned,
>that I can see, is -ENOSPC, which should never occur here since the original
>rules fitted in the first place.
>
>If it was possible to fail, then I think we would have a worse problem, in that
>deleting a single rule has wiped out our lpm table and left it in an
>inconsistent
>state, so the error handling probably needs to be better than just quitting.
>
>Finally, one other thing I spot looking through the code, is that there seems
>to
>be a worrying set of calls between add and delete. If the add function fails,
>then it calls delete which in turn will call add again, etc. etc. This may all
>work
>correctly, but it seems fragile and error prone to me - especially if we allow
>calls from one to another to fail.
>
>This looks like it might need some further examination to verify what the
>possible failure cases are and what happens in each scenario.
>
>Regards,
>/Bruce
Hi Bruce,
In my opinion the worst-case scenario should be take into account. If function
like rte_lpm6_add() returns false then it should be handled.
Anyway I agree with you that if the function fail then we have serious problem.
I see two problems:
1. Code construction: calls between function rte_lpm6_add() and
rte_lpm6_delete(). As you said it should be examined.
2. How we should handle situation if the rules table are not reconstructed
after delete operation.
I propose to add new issue in ClearQuest to proceed solve the problems because
there are extend the original issue (CID 13205 Unchecked return value) from
Coverity.
Regards,
S?awomir
