Fri, Oct 11, 2019 at 04:04:29AM CEST, jakub.kicin...@netronome.com wrote:
>On Thu, 10 Oct 2019 15:18:49 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <j...@mellanox.com>
>>
>> During health reporter operations, driver might want to fill-up
>> the extack message, so propagate extack down to the health reporter ops.
>>
>> Signed-off-by: Jiri Pirko <j...@mellanox.com>
>
>> @@ -507,11 +507,14 @@ enum devlink_health_reporter_state {
>> struct devlink_health_reporter_ops {
>> char *name;
>> int (*recover)(struct devlink_health_reporter *reporter,
>> - void *priv_ctx);
>> + void *priv_ctx, struct netlink_ext_ack *extack);
>> int (*dump)(struct devlink_health_reporter *reporter,
>> - struct devlink_fmsg *fmsg, void *priv_ctx);
>> + struct devlink_fmsg *fmsg, void *priv_ctx,
>> + struct netlink_ext_ack *extack);
>> int (*diagnose)(struct devlink_health_reporter *reporter,
>> - struct devlink_fmsg *fmsg);
>> + struct devlink_fmsg *fmsg,
>> + struct netlink_ext_ack *extack);
>> +
>
>nit: Looks like an extra new line snuck in here?
>
>> };
>>
>> /**
>
>> @@ -4946,11 +4947,12 @@ int devlink_health_report(struct
>> devlink_health_reporter *reporter,
>>
>> mutex_lock(&reporter->dump_lock);
>> /* store current dump of current error, for later analysis */
>> - devlink_health_do_dump(reporter, priv_ctx);
>> + devlink_health_do_dump(reporter, priv_ctx, NULL);
>> mutex_unlock(&reporter->dump_lock);
>>
>> 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?
>
>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).
>
>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].
>
>I wonder if this is a special case where outputting to the logs is a
>good idea? Really for all auto-recoverable health reporters the extack
>argument will just confuse driver authors. If driver uses extack here
>instead of printing to the logs information why auto-recovery failed is
>likely to get lost.
>
>Am I over-thinking this?
My gut feeling is kidn of odd about allocating some netlink specific
struct and use it separatelly from netlink :/
In any case, I think that this should probably be a separate patch/set.
>
>[1] https://www.spinics.net/lists/netdev/msg431998.html
