Re: [RFC 4/5] netlink: prepare validate extack setting for recursion

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: Thu, 20 Sep 2018 10:14:10 +0200	[thread overview]
Message-ID: <1537431250.3874.7.camel@sipsolutions.net> (raw)
In-Reply-To: <20180919211048.GN4590@localhost.localdomain> (sfid-20180919_231054_707223_F5EF1A9D)

On Wed, 2018-09-19 at 18:10 -0300, Marcelo Ricardo Leitner wrote:

> > FWIW, if you do think that there's a need for distinguishing this, then
> > I'd argue that perhaps the right way to address this would be to extend
> > this all the way to userspace and have two separate attributes for
> > errors and warnings in the extended ACK message?
> 
> Likely, yes. And perhaps even support multiple messages. That way we
> could, for example, parse all attributes and return a list of the all
> the offending ones, instead of returning one at a time. Net benefit?
> Not clear.. over engineering? Possibly.

Not sure I'd want to continue parsing after hitting something that's
considered garbage? It might be that the attribute length field is
corrupted and the data is actually fine, or something, and then
continuing to parse would just lead to more errors over and over again.

Also, I'd worry about being able to "blow up" the message, if we get a
text & bad attribute for everything that's wrong, it's pretty easy to
create a sort of "message amplification" and we'd probably have to
defend against that in terms of limiting memory allocation for all the
possible errors etc. Not sure I'd want to go there.

> I agree with you that in general we should not need this.

:-)

> > I'm still not really sure what the use case for a warning is, so not
> > sure I can really comment on this.
> 
> Good point. From iproute POV, a warning is a non-fatal message. From
> an user perspective, that probably translates are nothing because in
> the end the command actually worked. :-)
> 
> Seriously, I do think it's more of a hint for developers than anyone
> else.

I guess we also have the (ratelimited) messages from the kernel, like
the one saying you have extra bytes after your attributes at the end of
the message. Not sure which makes more sense, depends on the specific
case you'd use this in I guess.

Anyway - we got into this discussion because of all the extra recursion
stuff I was adding. With the change suggested by David we don't need
that now at all, so I guess it'd be better to propose a patch if you (or
perhaps I will see a need later) need such a facility for multiple
messages or multiple message levels?

johannes