On Thu, 2019-10-10 at 19:04 -0700, Jakub Kicinski wrote:
> > if (reporter->auto_recover)
> > - return devlink_health_reporter_recover(reporter, priv_ctx);
> > + return devlink_health_reporter_recover(reporter,
> > + priv_ctx, NULL);
> >
> > return 0;
> > }
>
> Thinking about this again - would it be entirely insane to allocate the
> extack on the stack here? And if anything gets set output into the logs?
I think that's fine.
> For context the situation here is that the health API can be poked from
> user space, but also the recovery actions are triggered automatically
> when failure is detected, if so configured (usually we expect them to
> be).
Right. We have similar situations in the wireless stack, and we usually
just get very noisy & WARN. But then it's a level lower, so you don't
have any extack around there :)
> When we were adding the extack helper for the drivers to use Johannes
> was concerned about printing to logs because that gave us a
> disincentive to convert all locations, and people could get surprised
> by the logs disappearing when more places are converted to extack [1].
Yes, but that argument doesn't apply here. The argument was around code
like
> +#define NL_SET_ERR_MSG(extack, msg) do { \
> + struct netlink_ext_ack *_extack = (extack); \
> + static const char _msg[] = (msg); \
> + \
> + if (_extack) \
> + _extack->_msg = _msg; \
> + else \
> + pr_info("%s\n", _msg); \
> } while (0)
(which I quoted in the message you linked).
I still stand by my comment there, I think it'd be a bad idea to do
this, because it'd mean that some random code using NL_SET_ERR_MSG()
might print something to the log if it's called in one way, and perhaps
the fact that it's getting NULL there is due to some higher level call
chain a few steps up "losing" the extack (or not having one), but then
if that's fixed suddenly all the messages disappear.
In the case you're proposing it's entirely different - you're proposing
that a non-netlink caller still supplies an extack that can be filled,
and then prints it out by itself. There's no reason to believe that
would be changed to suddenly have a real netlink extack that *doesn't*
get printed - that wouldn't make sense since you're doing this precisely
because you're *not* inside a netlink call.
Now, if you were saying "let's have a netlink handler that prints the
extack because a few levels up in the callstack we aren't passing the
parameter down yet" I'd still oppose it on similar grounds - that's
something we'd want to fix later to actually report to userspace - but
here there's no chance of that.
So to me this looks fine.
I don't really share the concern about extack being netlink specific and
then using it here - it ultimately doesn't matter whether this thing is
called "netlink_extack" or "extended_error_reporting", IMHO.
I guess we could wrap this so we have
struct devlink_error_report {
/* pretty much identical content to extack? */
};
and then in this path just print it, while in the actual netlink paths
convert it to extack a level up. That might be the right thing from an
abstraction POV (if we were designing this in Enterprise Architect ;-) )
but pragmatically I'd say it doesn't really matter what the struct is
called, and extack already has helper macros etc.
johannes
