Re: [PATCH RFC iproute2] libnetlink: allow reading more than one message from extack

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

On Mon, Mar 19, 2018 at 09:09:50AM -0700, Stephen Hemminger wrote:
> On Fri, 16 Mar 2018 16:23:09 -0300
> Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> wrote:
> 
> > This patch introduces support for reading more than one message from
> > extack's and to adjust their level (warning/error) accordingly.
> > 
> > Yes, there is a FIXME tag in the callback call for now.
> > 
> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 
> Make sense, can hold off until kernel supports warnings.

Okay.

> 
> > ---
> >  lib/libnetlink.c | 55 ++++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 40 insertions(+), 15 deletions(-)
> > 
> > diff --git a/lib/libnetlink.c b/lib/libnetlink.c
> > index 928de1dd16d84b7802c06d0a659f5d73e1bbcb4b..7c4c81e11e02ea857888190eb5e7a9e99d159bb3 100644
> > --- a/lib/libnetlink.c
> > +++ b/lib/libnetlink.c
> > @@ -43,9 +43,16 @@ static const enum mnl_attr_data_type extack_policy[NLMSGERR_ATTR_MAX + 1] = {
> >  	[NLMSGERR_ATTR_OFFS]	= MNL_TYPE_U32,
> >  };
> >  
> > +#define NETLINK_MAX_EXTACK_MSGS 8
> 
> Would rather not have fixed maximums

Makes sense. Then it is more forward-compatible in case we increase
that on kernel side later.

> 
> > +struct extack_args {
> > +	const struct nlattr *msg[NETLINK_MAX_EXTACK_MSGS];
> > +	const struct nlattr *offs;
> > +	int msg_count;
> > +};
> 
> If you put msg[] last in structure, it could be variable length.

It will also require a two-times parsing then because we don't have an
attribute with the number of messages in there and we have to allocate
it with a certain length.

> 
> >  static int err_attr_cb(const struct nlattr *attr, void *data)
> >  {
> > -	const struct nlattr **tb = data;
> > +	struct extack_args *tb = data;
> >  	uint16_t type;
> >  
> >  	if (mnl_attr_type_valid(attr, NLMSGERR_ATTR_MAX) < 0) {
> > @@ -60,19 +67,23 @@ static int err_attr_cb(const struct nlattr *attr, void *data)
> >  		return MNL_CB_ERROR;
> >  	}
> >  
> > -	tb[type] = attr;
> > +	if (type == NLMSGERR_ATTR_OFFS)
> > +		tb->offs = attr;
> > +	else if (tb->msg_count < NETLINK_MAX_EXTACK_MSGS)
> > +		tb->msg[tb->msg_count++] = attr;
> >  	return MNL_CB_OK;
> >  }
> >  
> >  /* dump netlink extended ack error message */
> >  int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
> >  {
> > -	struct nlattr *tb[NLMSGERR_ATTR_MAX + 1] = {};
> > +	struct extack_args tb = {};
> >  	const struct nlmsgerr *err = mnl_nlmsg_get_payload(nlh);
> >  	const struct nlmsghdr *err_nlh = NULL;
> >  	unsigned int hlen = sizeof(*err);
> > -	const char *msg = NULL;
> > +	const char *msg[NETLINK_MAX_EXTACK_MSGS] = {};
> >  	uint32_t off = 0;
> > +	int ret, i;
> >  
> >  	/* no TLVs, nothing to do here */
> >  	if (!(nlh->nlmsg_flags & NLM_F_ACK_TLVS))
> > @@ -82,14 +93,14 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
> >  	if (!(nlh->nlmsg_flags & NLM_F_CAPPED))
> >  		hlen += mnl_nlmsg_get_payload_len(&err->msg);
> >  
> > -	if (mnl_attr_parse(nlh, hlen, err_attr_cb, tb) != MNL_CB_OK)
> > +	if (mnl_attr_parse(nlh, hlen, err_attr_cb, &tb) != MNL_CB_OK)
> >  		return 0;
> >  
> > -	if (tb[NLMSGERR_ATTR_MSG])
> > -		msg = mnl_attr_get_str(tb[NLMSGERR_ATTR_MSG]);
> > +	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && tb.msg[i]; i++)
> > +		msg[i] = mnl_attr_get_str(tb.msg[i]);
> >  
> > -	if (tb[NLMSGERR_ATTR_OFFS]) {
> > -		off = mnl_attr_get_u32(tb[NLMSGERR_ATTR_OFFS]);
> > +	if (tb.offs) {
> > +		off = mnl_attr_get_u32(tb.offs);
> >  
> >  		if (off > nlh->nlmsg_len) {
> >  			fprintf(stderr,
> > @@ -100,21 +111,35 @@ int nl_dump_ext_ack(const struct nlmsghdr *nlh, nl_ext_ack_fn_t errfn)
> >  	}
> >  
> >  	if (errfn)
> > -		return errfn(msg, off, err_nlh);
> > +		return errfn(*msg, off, err_nlh); /* FIXME */
> >  
> > -	if (msg && *msg != '\0') {
> > +	ret = 0;
> > +	for (i = 0; i < NETLINK_MAX_EXTACK_MSGS && msg[i]; i++) {
> >  		bool is_err = !!err->error;
> > +		const char *_msg = msg[i];
> > +
> > +		/* Message tagging has precedence.
> > +		 * KERN_WARNING = ASCII Start Of Header ('\001') + '4'
> > +		 */
> > +		if (!strncmp(_msg, "\0014", 2)) {
> > +			is_err = false;
> > +			_msg += 2;
> > +		}
> 
> If you are going to have an API that has levels, it must be the same
> as existing syslog kernel log format and maybe even get some code reuse.

It is the same representation already. The magic "\0014" in there is
the same as KERN_WARNING. I'll check how other userspace applications
are dealing with this, as I don't see KERN_WARNING being exported to
userspace.

Thanks,
  Marcelo

> 
> > +		/* But we can't have Error if it didn't fail. */
> > +		if (is_err && !err->error)
> > +			is_err = 0;
> >  
> >  		fprintf(stderr, "%s: %s",
> > -			is_err ? "Error" : "Warning", msg);
> > -		if (msg[strlen(msg) - 1] != '.')
> > +			is_err ? "Error" : "Warning", _msg);
> > +		if (_msg[strlen(_msg) - 1] != '.')
> >  			fprintf(stderr, ".");
> >  		fprintf(stderr, "\n");
> >  
> > -		return is_err ? 1 : 0;
> > +		if (is_err)
> > +			ret = 1;
> >  	}
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  #else
> >  #warning "libmnl required for error support"
>