Message-ID: <1537349117.10305.25.camel@sipsolutions.net>
Date:   Wed, 19 Sep 2018 11:25:17 +0200
From:   Johannes Berg <johannes@...solutions.net>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

On Wed, 2018-09-19 at 00:37 -0300, Marcelo Ricardo Leitner wrote:

> Did you consider indicating the message level, and only overwrite the
> message that is already in there if the new message level is higher
> than the current one?

Hmm, no, I guess I didn't - I'm not even sure I understand what you're
saying.

This code in itself generates no "warning" messages; that was just a
construct we discussed in the NLA_REJECT thread, e.g. if you say (like I
just also wrote in my reply to Jiri):

	NL_SET_ERR_MSG(extack, "warning: deprecated command");
	err = nla_parse(..., extack);
	if (err)
		return err;
	/* do something */
	return 0;

Here you could consider the message there a warning that's transported
out even if we return 0, but if we return with a failure from
nla_parse() (or nla_validate instead if you wish), then that failure
message "wins".

> This way the first to set an Error message will have it, and Warning
> messages would be overwritten by such if needed. But it also would
> cause the first warning to be held, and not the last one, as it does
> today. We want the first error, but the last warning otherwise.
> 
> It would not be possible to overwrite if new_msglvl >= cur_msglvl
> because then it would trigger the initial issue again, so some extra
> logic would be needed to solve this.

That sounds way more complex than what I'm doing now?

Note, like I said above, this isn't *generic* in any way. This code here
will only ever set error messages that should "win".

I suppose we could - technically - make that generic, in that we could
have both

  NLA_SET_WARN_MSG(extack, "...");
  NLA_SET_ERR_MSG(extack, "...");

and keep track of warning vs. error; however, just like my first version
of the NLA_REJECT patch, that would break existing code.

I also don't think that we actually *need* this complexity in general.
It should almost always be possible (and realistically, pretty easy) to
structure your code in a way that warning messages only go out if no
error message overwrites them. The only reason we were ever even
discussing this was that in NLA_REJECT I missed the fact that somebody
could've set a message before and thus would keep it rather than
overwrite it, which was a change in behaviour.

Now, with this patch, all I'm doing is changing the internal behaviour
of nla_parse/nla_validate - externally, it still overwrites any existing
message if an error occurs, but internally it keeps the inner-most
error.

Why is this? Consider this:

static const struct nla_policy inner_policy[] = {
	[INNER_FLAG] = { .type = NLA_REJECT,
                         .validation_data = "must not set this flag" }
};

static const struct nla_policy outer_policy[] = {
	[OUTER_NESTING] = { .type = NLA_NESTED, .len = INNER_MAX,
                            .validation_data = inner_policy,
};

Now if you invoke nla_parse/nla_validate with a message like this

[ OUTER_NESTING => [ INNER_FLAG, ... ], ... ]

you'd get "must not set this flag" with the error offset pointing to
that; if I didn't do this construction here with inner messages winning,
you'd get "Attribute failed policy validation" with the error offset
pointing to the "OUTER_NESTING" attribute, that's pretty useless.

>From an external API POV though, nla_validate/nla_parse will continue to
unconditionally overwrite any existing "warning" messages with errors,
if such occurred. They just won't overwrite their own messages when
returning from a nested policy validation.

johannes

Powered by blists - more mailing lists