Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote:
> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
> > Currently extack can carry only a single message, which is usually the
> > error message.
> > 
> > This imposes a limitation on a more verbose error reporting. For
> > example, it's not able to carry warning messages together with the error
> > message, or 2 warning messages.
> 
> 
> The only means for userspace to separate an error message from info or
> warnings is the error in nlmsgerr. If it is non-0, any extack message is
> considered an error else it is a warning.

I don't see your point here.

The proposed patch extends what you said to:
- include warnings on error reports
- allow more than 1 message

With the proposed patch, if nlmsgerr is 0 all messages are considered
as warnings. If it's non-zero, some may be marked as warnings.

AFAICT it is not far from what you described, and still honouring the
main knob, mlmsgerr.

> 
> 
> > 
> > One use case is when dealing with tc offloading. If it failed to
> > offload, and also failed to install on software, it will report only
> > regarding the error about the software datapath, but the error from the
> > hardware path would also have been welcomed.
> > 
> > This patch extends extack so it now can carry up to 8 messages and these
> > messages may be prefixed similarly to printk/pr_warning, so thus they
> > can be tagged either was warning or error.
> > 
> > Fixed number of messages because supporting a dynamic limit seem to be
> > an overkill for the moment. Remember that this is not meant to be a
> > trace tool, but an error reporting one.
> > 
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > ---
> >  include/linux/netlink.h  | 50 +++++++++++++++++++++++++++++-------------------
> >  net/netlink/af_netlink.c | 12 +++++++-----
> >  2 files changed, 37 insertions(+), 25 deletions(-)
> > 
> > diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> > index f3075d6c7e8229c999ab650537f1e3b11e1f457b..d9780836cf263d4c436d732e9b7a8cde0739ac23 100644
> > --- a/include/linux/netlink.h
> > +++ b/include/linux/netlink.h
> > @@ -71,43 +71,53 @@ netlink_kernel_create(struct net *net, int unit, struct netlink_kernel_cfg *cfg)
> >   * @cookie: cookie data to return to userspace (for success)
> >   * @cookie_len: actual cookie data length
> >   */
> > +#define NETLINK_MAX_EXTACK_MSGS 8
> 
> 8 is way too many. If some change fails because of an error, why would a

Ok. I'm fine with 4 for now.

> single error message not be enough? If it is a not an error, why is more
> than 1 warning message not enough? (I forget the details of the tc
> 'skip_sw' use case)

Because 1 message assumes you have a simple and linear code path being
executed and that it aborts on the first error it encounters.

You could, for example, report several bad arguments at once instead
of having the user to fix & retry until he gets it all right.

You can also have situations like:
Warning: option foo is useless with option bar specified.
    (as in: the rule may not work as you intended)
Error: failed to allocate a new node.

The goal is to be able to deliver more information to the
user/software using netlink and be able to log it, for later analysis.

If you have a look at mlx5/core/en_tc.c, there are several
printk(KERN_WARNING or pr_warn calls that should be getting returned
via extack and not to kernel dmesg. That's just one domain and it is
not aware if there is already a message stored in extack or if there
will be another one later.

> 
> 
> >  struct netlink_ext_ack {
> > -	const char *_msg;
> > +	const char *_msg[NETLINK_MAX_EXTACK_MSGS];
> >  	const struct nlattr *bad_attr;
> >  	u8 cookie[NETLINK_MAX_COOKIE_LEN];
> >  	u8 cookie_len;
> > +	u8 _msg_count;
> >  };
> >  
> > -/* Always use this macro, this allows later putting the
> > - * message into a separate section or such for things
> > - * like translation or listing all possible messages.
> > - * Currently string formatting is not supported (due
> > - * to the lack of an output buffer.)
> > +/* Always use these macros, this allows later putting
> > + * the message into a separate section or such for
> > + * things like translation or listing all possible
> > + * messages.  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;		\
> > -	struct netlink_ext_ack *__extack = (extack);	\
> > -							\
> > -	if (__extack)					\
> > -		__extack->_msg = __msg;			\
> > +#define NL_SET_MSG(extack, msg) do {					\
> > +	static const char __msg[] = msg;				\
> > +	struct netlink_ext_ack *__extack = (extack);			\
> > +									\
> > +	if (__extack &&							\
> > +	    !WARN_ON(__extack->_msg_count >= NETLINK_MAX_EXTACK_MSGS))	\
> > +		__extack->_msg[__extack->_msg_count++] = __msg;		\
> >  } while (0)
> >  
> > +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
> > +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
> > +
> >  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
> >  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
> > +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
> > +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
> > +
> 
> Adding separate macros for error versus warning is confusing since from
> an extack perspective a message is a message and there is no uapi to
> separate them.

Are you saying the markings at beginning of the messages are not
possible? If that's the case, we probably can think of something else,
as I see value in being able to deliver warnings together with errors.

  M.