From: Johannes Berg <johannes@sipsolutions•net>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail•com>
Cc: netdev@vger•kernel.org
Subject: Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
Date: Wed, 19 Sep 2018 11:25:17 +0200 [thread overview]
Message-ID: <1537349117.10305.25.camel@sipsolutions.net> (raw)
In-Reply-To: <20180919033733.GK4590@localhost.localdomain> (sfid-20180919_053738_857743_037C8688)
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
next prev parent reply other threads:[~2018-09-19 15:02 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-18 13:12 [RFC 1/5] netlink: remove NLA_NESTED_COMPAT Johannes Berg
2018-09-18 13:12 ` [RFC 2/5] netlink: set extack error message in nla_validate() Johannes Berg
2018-09-18 17:18 ` David Ahern
2018-09-18 17:36 ` Johannes Berg
2018-09-18 13:12 ` [RFC 3/5] netlink: combine validate/parse functions Johannes Berg
2018-09-18 13:12 ` [RFC 4/5] netlink: prepare validate extack setting for recursion Johannes Berg
2018-09-19 3:37 ` Marcelo Ricardo Leitner
2018-09-19 9:25 ` Johannes Berg [this message]
2018-09-19 9:44 ` Jiri Benc
2018-09-19 18:46 ` Marcelo Ricardo Leitner
2018-09-19 19:19 ` Johannes Berg
2018-09-19 21:10 ` Marcelo Ricardo Leitner
2018-09-20 8:14 ` Johannes Berg
2018-09-20 17:48 ` Marcelo Ricardo Leitner
2018-09-19 9:10 ` Jiri Benc
2018-09-19 9:15 ` Johannes Berg
2018-09-19 9:28 ` Jiri Benc
2018-09-19 9:44 ` Johannes Berg
2018-09-18 13:12 ` [RFC 5/5] netlink: allow NLA_NESTED to specify nested policy to validate Johannes Berg
2018-09-18 17:18 ` [RFC 1/5] netlink: remove NLA_NESTED_COMPAT David Ahern
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1537349117.10305.25.camel@sipsolutions.net \
--to=johannes@sipsolutions$(echo .)net \
--cc=marcelo.leitner@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox