From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 822372BB9 for ; Fri, 6 May 2016 14:25:58 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 06 May 2016 05:25:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,587,1455004800"; d="scan'208";a="947568119" Received: from irsmsx106.ger.corp.intel.com ([163.33.3.31]) by orsmga001.jf.intel.com with ESMTP; 06 May 2016 05:25:51 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX106.ger.corp.intel.com (163.33.3.31) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 6 May 2016 13:25:50 +0100 Received: from irsmsx103.ger.corp.intel.com ([169.254.3.54]) by IRSMSX156.ger.corp.intel.com ([169.254.3.174]) with mapi id 14.03.0248.002; Fri, 6 May 2016 13:25:50 +0100 From: "Mrozowicz, SlawomirX" To: "Richardson, Bruce" CC: "dev@dpdk.org" Thread-Topic: [PATCH] lpm: unchecked return value Thread-Index: AQHRoILr14kLAkBepk+dGXM4f49ZSJ+nP+UAgASeDnA= Date: Fri, 6 May 2016 12:25:49 +0000 Message-ID: <158888A50F43E34AAE179517F56C97455A4043@IRSMSX103.ger.corp.intel.com> References: <1461761554-5900-1-git-send-email-slawomirx.mrozowicz@intel.com> <20160503143404.GA22728@bricha3-MOBL3> In-Reply-To: <20160503143404.GA22728@bricha3-MOBL3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="iso-8859-2" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] lpm: unchecked return value X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 06 May 2016 12:25:59 -0000 >-----Original Message----- >From: Richardson, Bruce >Sent: Tuesday, May 3, 2016 4:34 PM >To: Mrozowicz, SlawomirX >Cc: dev@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 >> --- >> 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 =3D 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 =3D 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 =3D 0; i < lpm->used_rules && status >=3D 0; i++) { >> + status =3D 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 t= he >lpm6_add calls can fail in this instance. Looking through the code, this f= unction >deletes the rule and then clears the actual lpm lookup tables before re-ad= ding >all other routes to it again. The only error condition that could be retur= ned, >that I can see, is -ENOSPC, which should never occur here since the origin= al >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 incon= sistent >state, so the error handling probably needs to be better than just quittin= g. > >Finally, one other thing I spot looking through the code, is that there se= ems to >be a worrying set of calls between add and delete. If the add function fai= ls, >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 a= llow >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 funct= ion 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 prob= lem. I see two problems: 1. Code construction: calls between function rte_lpm6_add() and rte_lpm6_de= lete(). 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 beca= use there are extend the original issue (CID 13205 Unchecked return value) = from Coverity. Regards, S=B3awomir