From: Leon Romanovsky <leon@kernel.org> To: Jakub Kicinski <kuba@kernel.org> Cc: Steffen Klassert <steffen.klassert@secunet.com>, Veaceslav Falico <vfalico@gmail.com>, Herbert Xu <herbert@gondor.apana.org.au>, Jonathan Corbet <corbet@lwn.net>, Jay Vosburgh <j.vosburgh@gmail.com>, oss-drivers@corigine.com, linux-doc@vger.kernel.org, Raju Rangoju <rajur@chelsio.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>, Eric Dumazet <edumazet@google.com>, netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org, Ayush Sawal <ayush.sawal@chelsio.com>, Simon Horman <simon.horman@corigine.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, Paolo Abeni <pabeni@redhat.com>, Saeed Mahameed <saeedm@nvidia.com>, "David S . Miller" <davem@davemloft.net>, Andy Gospodarek <andy@greyhouse.net> Subject: Re: [Intel-wired-lan] [PATCH net-next v1 01/10] xfrm: extend add policy callback to set failure reason Date: Thu, 26 Jan 2023 09:28:02 +0200 [thread overview] Message-ID: <Y9Irgrgf3uxOjwUm@unreal> (raw) In-Reply-To: <20230125110226.66dc7eeb@kernel.org> On Wed, Jan 25, 2023 at 11:02:26AM -0800, Jakub Kicinski wrote: > On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote: > > - err = dev->xfrmdev_ops->xdo_dev_policy_add(xp); > > + err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack); > > if (err) { > > xdo->dev = NULL; > > xdo->real_dev = NULL; > > xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED; > > xdo->dir = 0; > > netdev_put(dev, &xdo->dev_tracker); > > - NL_SET_ERR_MSG(extack, "Device failed to offload this policy"); > > In a handful of places we do: > > if (!extack->msg) > NL_SET_ERR_MSG(extack, "Device failed to offload this policy"); > > in case the device did not provide the extack. > Dunno if it's worth doing here. Honestly, I followed devlink.c which didn't do that, but looked again and found that devlink can potentially overwrite messages :) For example in this case: 997 err = ops->port_fn_state_get(port, &state, &opstate, extack); 998 if (err) { 999 if (err == -EOPNOTSUPP) 1000 return 0; 1001 return err; 1002 } 1003 if (!devlink_port_fn_state_valid(state)) { 1004 WARN_ON_ONCE(1); 1005 NL_SET_ERR_MSG_MOD(extack, "Invalid state read from driver"); 1006 return -EINVAL; 1007 } So what do you think about the following change, so we can leave NL_SET_ERR_MSG_MOD() in devlink and xfrm intact? diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 38f6334f408c..d6f3a958e30b 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -101,7 +101,7 @@ struct netlink_ext_ack { \ do_trace_netlink_extack(__msg); \ \ - if (__extack) \ + if (__extack && !__extack->msg) \ __extack->_msg = __msg; \ } while (0) @@ -111,7 +111,7 @@ struct netlink_ext_ack { #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do { \ struct netlink_ext_ack *__extack = (extack); \ \ - if (!__extack) \ + if (!__extack || __extack->msg) \ break; \ if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN, \ "%s" fmt "%s", "", ##args, "") >= \ _______________________________________________ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
WARNING: multiple messages have this Message-ID (diff)
From: Leon Romanovsky <leon@kernel.org> To: Jakub Kicinski <kuba@kernel.org> Cc: "David S . Miller" <davem@davemloft.net>, Steffen Klassert <steffen.klassert@secunet.com>, Andy Gospodarek <andy@greyhouse.net>, Ayush Sawal <ayush.sawal@chelsio.com>, Eric Dumazet <edumazet@google.com>, Herbert Xu <herbert@gondor.apana.org.au>, intel-wired-lan@lists.osuosl.org, Jay Vosburgh <j.vosburgh@gmail.com>, Jesse Brandeburg <jesse.brandeburg@intel.com>, Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org, netdev@vger.kernel.org, oss-drivers@corigine.com, Paolo Abeni <pabeni@redhat.com>, Raju Rangoju <rajur@chelsio.com>, Saeed Mahameed <saeedm@nvidia.com>, Simon Horman <simon.horman@corigine.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, Veaceslav Falico <vfalico@gmail.com> Subject: Re: [PATCH net-next v1 01/10] xfrm: extend add policy callback to set failure reason Date: Thu, 26 Jan 2023 09:28:02 +0200 [thread overview] Message-ID: <Y9Irgrgf3uxOjwUm@unreal> (raw) In-Reply-To: <20230125110226.66dc7eeb@kernel.org> On Wed, Jan 25, 2023 at 11:02:26AM -0800, Jakub Kicinski wrote: > On Tue, 24 Jan 2023 13:54:57 +0200 Leon Romanovsky wrote: > > - err = dev->xfrmdev_ops->xdo_dev_policy_add(xp); > > + err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack); > > if (err) { > > xdo->dev = NULL; > > xdo->real_dev = NULL; > > xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED; > > xdo->dir = 0; > > netdev_put(dev, &xdo->dev_tracker); > > - NL_SET_ERR_MSG(extack, "Device failed to offload this policy"); > > In a handful of places we do: > > if (!extack->msg) > NL_SET_ERR_MSG(extack, "Device failed to offload this policy"); > > in case the device did not provide the extack. > Dunno if it's worth doing here. Honestly, I followed devlink.c which didn't do that, but looked again and found that devlink can potentially overwrite messages :) For example in this case: 997 err = ops->port_fn_state_get(port, &state, &opstate, extack); 998 if (err) { 999 if (err == -EOPNOTSUPP) 1000 return 0; 1001 return err; 1002 } 1003 if (!devlink_port_fn_state_valid(state)) { 1004 WARN_ON_ONCE(1); 1005 NL_SET_ERR_MSG_MOD(extack, "Invalid state read from driver"); 1006 return -EINVAL; 1007 } So what do you think about the following change, so we can leave NL_SET_ERR_MSG_MOD() in devlink and xfrm intact? diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 38f6334f408c..d6f3a958e30b 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -101,7 +101,7 @@ struct netlink_ext_ack { \ do_trace_netlink_extack(__msg); \ \ - if (__extack) \ + if (__extack && !__extack->msg) \ __extack->_msg = __msg; \ } while (0) @@ -111,7 +111,7 @@ struct netlink_ext_ack { #define NL_SET_ERR_MSG_FMT(extack, fmt, args...) do { \ struct netlink_ext_ack *__extack = (extack); \ \ - if (!__extack) \ + if (!__extack || __extack->msg) \ break; \ if (snprintf(__extack->_msg_buf, NETLINK_MAX_FMTMSG_LEN, \ "%s" fmt "%s", "", ##args, "") >= \
next prev parent reply other threads:[~2023-01-26 7:28 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-24 11:54 [PATCH net-next v1 00/10] Convert drivers to return XFRM configuration errors through extack Leon Romanovsky 2023-01-24 11:54 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:54 ` [PATCH net-next v1 01/10] xfrm: extend add policy callback to set failure reason Leon Romanovsky 2023-01-24 11:54 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-25 19:02 ` Jakub Kicinski 2023-01-25 19:02 ` [Intel-wired-lan] " Jakub Kicinski 2023-01-26 7:28 ` Leon Romanovsky [this message] 2023-01-26 7:28 ` Leon Romanovsky 2023-01-26 9:45 ` Paolo Abeni 2023-01-26 9:45 ` [Intel-wired-lan] " Paolo Abeni 2023-01-26 10:42 ` Leon Romanovsky 2023-01-26 10:42 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-26 19:17 ` Leon Romanovsky 2023-01-26 19:17 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:54 ` [PATCH net-next v1 02/10] net/mlx5e: Fill IPsec policy validation " Leon Romanovsky 2023-01-24 11:54 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:54 ` [PATCH net-next v1 03/10] xfrm: extend add state callback to set " Leon Romanovsky 2023-01-24 11:54 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:55 ` [PATCH net-next v1 04/10] net/mlx5e: Fill IPsec state validation " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:55 ` [PATCH net-next v1 05/10] netdevsim: " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] [PATCH net-next v1 06/10] nfp: fill " Leon Romanovsky 2023-01-24 11:55 ` Leon Romanovsky 2023-01-24 11:55 ` [PATCH net-next v1 07/10] ixgbevf: " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:55 ` [PATCH net-next v1 08/10] ixgbe: " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:55 ` [PATCH net-next v1 09/10] bonding: " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] " Leon Romanovsky 2023-01-24 11:55 ` [Intel-wired-lan] [PATCH net-next v1 10/10] cxgb4: " Leon Romanovsky 2023-01-24 11:55 ` Leon Romanovsky 2023-01-25 19:01 ` [PATCH net-next v1 00/10] Convert drivers to return XFRM configuration errors through extack Jakub Kicinski 2023-01-25 19:01 ` [Intel-wired-lan] " Jakub Kicinski 2023-01-26 9:43 ` Steffen Klassert 2023-01-26 9:43 ` [Intel-wired-lan] " Steffen Klassert 2023-01-27 0:50 ` patchwork-bot+netdevbpf 2023-01-27 0:50 ` [Intel-wired-lan] " patchwork-bot+netdevbpf
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Y9Irgrgf3uxOjwUm@unreal \ --to=leon@kernel.org \ --cc=andy@greyhouse.net \ --cc=anthony.l.nguyen@intel.com \ --cc=ayush.sawal@chelsio.com \ --cc=corbet@lwn.net \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=herbert@gondor.apana.org.au \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=j.vosburgh@gmail.com \ --cc=jesse.brandeburg@intel.com \ --cc=kuba@kernel.org \ --cc=linux-doc@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=oss-drivers@corigine.com \ --cc=pabeni@redhat.com \ --cc=rajur@chelsio.com \ --cc=saeedm@nvidia.com \ --cc=simon.horman@corigine.com \ --cc=steffen.klassert@secunet.com \ --cc=vfalico@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.