From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pablo Neira Ayuso Subject: Re: [PATCH nf-next 1/1] netfilter: nf_tables: Refine the codes to eliminate useless condition checks in nf_tables_api.c Date: Mon, 16 Jan 2017 14:38:18 +0100 Message-ID: <20170116133818.GA5335@salvia> References: <1484098335-13300-1-git-send-email-fgao@ikuai8.com> <20170112112142.GA1666@salvia> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Netfilter Developer Mailing List To: Gao Feng Return-path: Received: from mail.us.es ([193.147.175.20]:33012 "EHLO mail.us.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751261AbdAPNi1 (ORCPT ); Mon, 16 Jan 2017 08:38:27 -0500 Received: from antivirus1-rhel7.int (unknown [192.168.2.11]) by mail.us.es (Postfix) with ESMTP id BBB2E392E0F for ; Mon, 16 Jan 2017 14:38:23 +0100 (CET) Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id 9C2FE8E62C for ; Mon, 16 Jan 2017 14:38:23 +0100 (CET) Received: from antivirus1-rhel7.int (localhost [127.0.0.1]) by antivirus1-rhel7.int (Postfix) with ESMTP id D34C49617D for ; Mon, 16 Jan 2017 14:38:19 +0100 (CET) Content-Disposition: inline In-Reply-To: Sender: netfilter-devel-owner@vger.kernel.org List-ID: On Thu, Jan 12, 2017 at 11:10:11PM +0800, Gao Feng wrote: > Hi Pablo, > > On Thu, Jan 12, 2017 at 7:21 PM, Pablo Neira Ayuso wrote: > > On Wed, Jan 11, 2017 at 09:32:15AM +0800, fgao@ikuai8.com wrote: > >> From: Gao Feng > >> > >> The return value of nf_tables_table_lookup is valid pointer or one > >> pointer error. There are two cases totally. > >> case1: IS_ERR(table) is true, it would return the error or reset the > >> table as NULL, it is unnecessary to perform the latter check > >> "table != NULL". > >> case2: IS_ERR(obj) is false, the table is one valid pointer. It is also > >> unnecessary to perform that check. > >> The nf_tables_newset and nf_tables_newobj have same logic codes. > >> > >> In summary, we could move the block of condition check "table != NULL" > >> in the else block to eliminate the original condition checks. > >> > >> Signed-off-by: Gao Feng > >> --- > >> net/netfilter/nf_tables_api.c | 12 +++--------- > >> 1 file changed, 3 insertions(+), 9 deletions(-) > >> > >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > >> index a019a87..3d7267f 100644 > >> --- a/net/netfilter/nf_tables_api.c > >> +++ b/net/netfilter/nf_tables_api.c > >> @@ -697,9 +697,7 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, > >> if (PTR_ERR(table) != -ENOENT) > >> return PTR_ERR(table); > >> table = NULL; > > > > We follow up with table = NULL down the code, I think this breaks. > > Look at the following nf_tables_table_lookup codes, it won't return NULL. > It returns one valid table pointer or one error. > > static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi, > const struct nlattr *nla, > u8 genmask) > { > struct nft_table *table; > > if (nla == NULL) > return ERR_PTR(-EINVAL); > > table = nft_table_lookup(afi, nla, genmask); > if (table != NULL) > return table; > > return ERR_PTR(-ENOENT); > } > > When returns one error, IS_ERR(table) is true, it then returns error > or reset table as NULL. > Wehn returns a valid table pointer, IS_ERR(table) is false, we could > perform the latter codes like "if (nlh->nlmsg_flags & NLM_F_EXCL)" > directly. Right. Then, I think we can remove the unneccessary table = NULL assigment: @@ -697,9 +697,7 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk, if (PTR_ERR(table) != -ENOENT) return PTR_ERR(table); table = NULL; <----- this - } - - if (table != NULL) { + } else { if (nlh->nlmsg_flags & NLM_F_EXCL) return -EEXIST; if (nlh->nlmsg_flags & NLM_F_REPLACE)