| Message ID | 20170425080644.122536-2-jakub.kicinski@netronome.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2017-04-25 at 01:06 -0700, Jakub Kicinski wrote: > +#define NL_SET_ERR_MSG(extack, msg) do { \ > + struct netlink_ext_ack *_extack = (extack); \ > + static const char _msg[] = (msg); \ > + \ > + if (_extack) \ > + _extack->_msg = _msg; \ > + else \ > + pr_info("%s\n", _msg); \ > } while (0) That's a good point, I used it only for genetlink so far where it was guaranteed non-NULL. I'm not really sure about the printing though - I'd rather not people start relying on that and then we convert to have non-NULL and the message disappears as a result ... johannes
On 04/25/2017 10:06 AM, Jakub Kicinski wrote: > As we propagate extended ack reporting throughout various paths in > the kernel it may happen that the same function is called with the > extended ack parameter passed as NULL. Make the NL_SET_ERR_MSG() > macro simply print the message to the logs if that happens. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > include/linux/netlink.h | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/include/linux/netlink.h b/include/linux/netlink.h > index 8d2a8924705c..b59cfbf2e2c7 100644 > --- a/include/linux/netlink.h > +++ b/include/linux/netlink.h > @@ -86,10 +86,14 @@ struct netlink_ext_ack { > * Currently string formatting is not supported (due > * to the lack of an output buffer.) > */ > -#define NL_SET_ERR_MSG(extack, msg) do { \ > - static const char _msg[] = (msg); \ > - \ > - (extack)->_msg = _msg; \ > +#define NL_SET_ERR_MSG(extack, msg) do { \ > + struct netlink_ext_ack *_extack = (extack); \ > + static const char _msg[] = (msg); \ > + \ > + if (_extack) \ > + _extack->_msg = _msg; \ > + else \ > + pr_info("%s\n", _msg); \ > } while (0) > > extern void netlink_kernel_release(struct sock *sk); Probably makes sense to have a NL_MOD_SET_ERR_MSG(), which then also prepends a KBUILD_MODNAME ": " string to the message (similar to pr_*()), so that it's easier to identify whether the error came from a specific driver or rather common core code?
On Tue, 25 Apr 2017 10:13:34 +0200, Johannes Berg wrote: > On Tue, 2017-04-25 at 01:06 -0700, Jakub Kicinski wrote: > > > +#define NL_SET_ERR_MSG(extack, msg) do { \ > > + struct netlink_ext_ack *_extack = (extack); \ > > + static const char _msg[] = (msg); \ > > + \ > > + if (_extack) \ > > + _extack->_msg = _msg; \ > > + else \ > > + pr_info("%s\n", _msg); \ > > } while (0) > > That's a good point, I used it only for genetlink so far where it was > guaranteed non-NULL. > > I'm not really sure about the printing though - I'd rather not people > start relying on that and then we convert to have non-NULL and the > message disappears as a result ... Yes, agreed. I don't really know what to do about that one though :| One could argue people may already be depending on the messages which I'm converting in this series... On the other hand, that would be considering logs as part of the ABI which we don't want to do. I'm leaning towards dropping the else clause and never printing, that will add an incentive for people to convert more paths to provide the ext ack. Any thoughts on that?
On Tue, 2017-04-25 at 13:53 -0700, Jakub Kicinski wrote: > On Tue, 25 Apr 2017 10:13:34 +0200, Johannes Berg wrote: > > On Tue, 2017-04-25 at 01:06 -0700, Jakub Kicinski wrote: > > > > > +#define NL_SET_ERR_MSG(extack, msg) do { \ > > > + struct netlink_ext_ack *_extack = (extack); \ > > > + static const char _msg[] = (msg); \ > > > + \ > > > + if (_extack) \ > > > + _extack->_msg = _msg; \ > > > + else \ > > > + pr_info("%s\n", _msg); \ > > > } while (0) > I'm leaning towards dropping the else clause and never printing, that > will add an incentive for people to convert more paths to provide the > ext ack. Any thoughts on that? Personally, I'm happy with that. johannes
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 8d2a8924705c..b59cfbf2e2c7 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -86,10 +86,14 @@ struct netlink_ext_ack { * Currently string formatting is not supported (due * to the lack of an output buffer.) */ -#define NL_SET_ERR_MSG(extack, msg) do { \ - static const char _msg[] = (msg); \ - \ - (extack)->_msg = _msg; \ +#define NL_SET_ERR_MSG(extack, msg) do { \ + struct netlink_ext_ack *_extack = (extack); \ + static const char _msg[] = (msg); \ + \ + if (_extack) \ + _extack->_msg = _msg; \ + else \ + pr_info("%s\n", _msg); \ } while (0) extern void netlink_kernel_release(struct sock *sk);
As we propagate extended ack reporting throughout various paths in the kernel it may happen that the same function is called with the extended ack parameter passed as NULL. Make the NL_SET_ERR_MSG() macro simply print the message to the logs if that happens. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- include/linux/netlink.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)