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
