From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guillaume Nault Subject: Re: [PATCH v2 net-next 1/5] l2tp: fix switch default error handling in l2tp_nl_cmd_session_create() Date: Tue, 16 Jan 2018 11:24:14 +0100 Message-ID: <20180116102414.GG1422@alphalink.fr> References: <011d981a9164950a7ff20f590344cc207f2234f4.1515940731.git.lorenzo.bianconi@redhat.com> <20180115183358.GC1422@alphalink.fr> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "David S. Miller" , netdev@vger.kernel.org, James Chapman To: Lorenzo Bianconi Return-path: Received: from zimbra.alphalink.fr ([217.15.80.77]:46629 "EHLO zimbra.alphalink.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751487AbeAPKYR (ORCPT ); Tue, 16 Jan 2018 05:24:17 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Jan 15, 2018 at 10:18:53PM +0100, Lorenzo Bianconi wrote: > > On Sun, Jan 14, 2018 at 03:50:54PM +0100, Lorenzo Bianconi wrote: > >> Although this issue is harmless since that code path is protected by the > >> check on l2tp_nl_cmd_ops[]/l2tp_nl_cmd_ops[]->session_create(), fix error > >> handling for L2TP_PWTYPE_IP/default case in l2tp_nl_cmd_session_create() > >> > >> Signed-off-by: Lorenzo Bianconi > >> --- > >> net/l2tp/l2tp_netlink.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c > >> index e1ca29f79821..48b5bf30ec50 100644 > >> --- a/net/l2tp/l2tp_netlink.c > >> +++ b/net/l2tp/l2tp_netlink.c > >> @@ -635,7 +635,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf > >> case L2TP_PWTYPE_IP: > >> default: > >> ret = -EPROTONOSUPPORT; > >> - break; > >> + goto out_tunnel; > >> } > >> > > Not sure if this change is really worthwhile. As you noted, this is > > unreachable code. And this switch should better be removed entirely: > > it doesn't do anything for supported pseudo-wires. > > > > And if PWTYPE_ETH_VLAN were to be implemented, it should perform its > > configuration consistency checking in its own PW specific code, not in > > the genl handler. > > > > Personally I would prefer to not remove some code that could be useful > for a future implementation, but just fix it if it presents issues to > address. > I don't think this code can be useful in the future, quite the other way around. l2tp_nl_cmd_session_create() has to make sure that it can safely call ->session_create(), nothing more. Removing this code would force a new PW implementation to do its configuration checking at the right place. BTW, writing code speculatively is generally not a good idea. Such code can't be tested, and the future tends to be quite different from what was expeceted anyway.