From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 7A7885922 for ; Tue, 3 May 2016 16:34:08 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 03 May 2016 07:34:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,572,1455004800"; d="scan'208";a="96334455" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.220.29]) by fmsmga004.fm.intel.com with SMTP; 03 May 2016 07:34:05 -0700 Received: by (sSMTP sendmail emulation); Tue, 03 May 2016 15:34:05 +0025 Date: Tue, 3 May 2016 15:34:04 +0100 From: Bruce Richardson To: Slawomir Mrozowicz Cc: dev@dpdk.org Message-ID: <20160503143404.GA22728@bricha3-MOBL3> References: <1461761554-5900-1-git-send-email-slawomirx.mrozowicz@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461761554-5900-1-git-send-email-slawomirx.mrozowicz@intel.com> Organization: Intel Shannon Ltd. User-Agent: Mutt/1.5.23 (2014-03-12) 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: Tue, 03 May 2016 14:34:08 -0000 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 = 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