Re: [patch net-next v2 2/4] devlink: propagate extack down to health reporter ops

[Jiri Pirko <jiri@resnulli.us>]

Fri, Oct 11, 2019 at 04:04:29AM CEST, jakub.kicinski@netronome.com wrote:
>On Thu, 10 Oct 2019 15:18:49 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@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 <jiri@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