>-----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