> -----Original Message----- > From: Richardson, Bruce > Sent: Thursday, June 23, 2016 10:48 AM > To: Jastrzebski, MichalX K <michalx.k.jastrzebski at intel.com> > Cc: Azarewicz, PiotrX T <piotrx.t.azarewicz at intel.com>; Mrozowicz, > SlawomirX <slawomirx.mrozowicz at intel.com>; dev at dpdk.org > Subject: Re: [PATCH] lpm: unchecked return value > > On Thu, Jun 23, 2016 at 09:13:22AM +0100, Jastrzebski, MichalX K wrote: > > > -----Original Message----- > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Azarewicz, > PiotrX T > > > Sent: Thursday, May 12, 2016 1:20 PM > > > To: Mrozowicz, SlawomirX <slawomirx.mrozowicz at intel.com>; > Richardson, > > > Bruce <bruce.richardson at intel.com> > > > Cc: dev at dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH] lpm: unchecked return value > > > > > > Hi, > > > > > > I handle Coverity defect ID 13201. It is about unchecked return value > from > > > rte_lpm6_delete() instances in rte_lpm6_add() function. > > > Next I found this thread and I see that both defects (ID 13205 and ID > 13201) > > > may be resolved all together. > > > > > > > >> 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. > > > > > > I agree that -ENOSPC should never occur here. So rte_lpm6_add() > instance > > > should never fail here. > > > > > > Next I looked at rte_lpm6_add() and if rte_lpm6_delete() instances in it > > > may fail? > > > The only suspicious place that I found is place when add every rule > again > > > but that should work as discussed above. > > > > > > > > > > > > >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. > > > > > > I see no failure scenarios in here. I mean I see no possibility to create > test > > > that show that add function fail in del and opposite. > > > The only scenario what I have in my mind is that someone call add > or/and > > > del functions on different threads with the same lpm table instance, but > > > this is not allowed, cause we know that this functions are not thread > safe. > > > > > > > > > > > > >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 > > > > > > I propose to classify this Coverity issues (ID 13205 and ID 13201) as > > > Intentional. > > > > > > Regards, > > > Piotr > > > > Hi Bruce, > > We would like to move forward with theses Coverity defects thus > > Please share your opinion about classifying these defects as Intentional? > > > > Michal > > From previous analysis detailed above, it looks like there is no issue with > failing to check the return values here, so I'm ok with this classification. > > /Bruce
Thanks Bruce, There is a third issue in Coverity tool, not listed here but related with the same problem (CID13203) in rte_lpm6_delete_bulk_func function. We will also classify it as Intentional Michal.
