From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH v2] app/testpmd: fix memory leak Date: Mon, 30 Jan 2017 10:33:18 +0100 Message-ID: <20170130093318.GB10133@6wind.com> References: <1485522948-210951-1-git-send-email-pablo.de.lara.guarch@intel.com> <1485528898-53283-1-git-send-email-pablo.de.lara.guarch@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: jingjing.wu@intel.com, dev@dpdk.org To: Pablo de Lara Return-path: Received: from mail-wm0-f48.google.com (mail-wm0-f48.google.com [74.125.82.48]) by dpdk.org (Postfix) with ESMTP id A9C8A3989 for ; Mon, 30 Jan 2017 10:33:27 +0100 (CET) Received: by mail-wm0-f48.google.com with SMTP id c85so205890750wmi.1 for ; Mon, 30 Jan 2017 01:33:27 -0800 (PST) Content-Disposition: inline In-Reply-To: <1485528898-53283-1-git-send-email-pablo.de.lara.guarch@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Pablo, On Fri, Jan 27, 2017 at 02:54:58PM +0000, Pablo de Lara wrote: > Free memory when port flow entry creation fails. > > Coverity issue: 139600 > Fixes: 938a184a1870 ("app/testpmd: implement basic support for flow API") > > Signed-off-by: Pablo de Lara > --- > Changes in v2: > > - Removed unnecessary conditional > > app/test-pmd/config.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c > index 5834498..467932f 100644 > --- a/app/test-pmd/config.c > +++ b/app/test-pmd/config.c > @@ -954,6 +954,7 @@ port_flow_new(const struct rte_flow_attr *attr, > goto store; > } > notsup: > + free(pf); > rte_errno = err; > return NULL; > } > -- > 2.7.4 > I think this is a false positive, which is why I did not address it during the last round of Coverity issues. As a two-pass function, errors are checked during the first pass, when pf is not allocated yet. During the second pass, intermediate functions are not supposed to return a different result and "notsup" cannot occur. I think assert()/rte_assert() would make more sense and should fool Coverity into understanding the expected behavior. The commit log should reflect that we are addressing a false positive since there is no problem with the code logic (of course unless I missed anything obvious). Thanks. -- Adrien Mazarguil 6WIND