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

Jiri Pirko
2020-03-26T21:23:21Z

Thu, Mar 26, 2020 at 06:39:13PM CET, kuba@kernel.org wrote:
>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.

Agreed.

>
>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..