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 <lorenzo.bianc...@redhat.com> > >> --- > >> 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.
