Re: [PATCH net-next 2/2] devlink: Add auto dump flag to health reporter

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 26 Mar 2020 11:22:44 +0100 Jiri Pirko wrote:
> >> >>> @@ -4983,6 +4985,10 @@ devlink_nl_health_reporter_fill(struct sk_buff *msg,
> >> >>>   	    nla_put_u64_64bit(msg, DEVLINK_ATTR_HEALTH_REPORTER_DUMP_TS_NS,
> >> >>>   			      reporter->dump_real_ts, DEVLINK_ATTR_PAD))
> >> >>>   		goto reporter_nest_cancel;
> >> >>> +	if (reporter->ops->dump &&
> >> >>> +	    nla_put_u8(msg, DEVLINK_ATTR_HEALTH_REPORTER_AUTO_DUMP,
> >> >>> +		       reporter->auto_dump))
> >> >>> +		goto reporter_nest_cancel;    
> >> >>
> >> >> Since you're making it a u8 - does it make sense to indicate to user    
> >> > 
> >> > Please don't be mistaken. u8 carries a bool here.  
> >
> >Are you okay with limiting the value in the policy?  
> 
> Well, not-0 means true. Do you think it is wise to limit to 0/1?

Just future proofing, in general seems wise to always constrain the
input as much as possible. But in this case we already have similar
attrs in the dump which don't have the constraint, and we will probably
want consistency, so maybe we're unlikely to use other values.

In particular I was wondering if auto-dump value can be extended to
mean the number of dumps we want to collect, the current behavior I
think matches collecting just one. But obviously this can be solved
with a new attr when needed..