Re: [PATCH net-next 1/6] net: sched: sch_api: handle generic qdisc errors

[David Ahern <dsahern@gmail.com>]

On 12/7/17 5:04 AM, Jamal Hadi Salim wrote:
> On 17-12-07 12:28 AM, David Ahern wrote:
> 
>>>   -    err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, NULL);
>>> -    if (err < 0)
>>> +    err = nla_parse_nested(tb, TCA_STAB_MAX, opt, stab_policy, extack);
>>> +    if (err < 0) {
>>> +        NL_SET_ERR_MSG(extack, "Failed to parse stab netlink msg");
>>
>> you don't want to set extack here; it should be set in nla_parse_nested
>> since it is passed as an arg.
>>
> 
> Can you really have a generic message in nla_parse(_nested)? What
> would it say?
> Note: the bad attribute is saved in the bowels of nla_parsing.

sure, nla_parse only sets the bad attr arg. If you keep the above, then
it needs to be corrected - it is failing to parse the TCA_STAB nested
attribute.
> 
> -
>>>       stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL);
>>> -    if (!stab)
>>> +    if (!stab) {
>>> +        NL_SET_ERR_MSG(extack, "Failed to allocate memory for stab
>>> data");
>>
>> ENOMEM does not need a text message. Which memory allocation failed is
>> not really relevant.
>>
> 
> YMMV.
> On the one hand it is useful to distinguish which allocation
> in the code path failed(if there was a bug for example).
> On the other hand you could argue an alloc failure is as dramatic
> as the "sky is falling" - doesnt matter what part of the globe it is
> falling at. If the cost of sending or not is the same why not include
> the message?

What value are the messages providing above and beyond the standard libc
strerror(errno)? In the case of ENOMEM, there is nothing in the user can
do to fix that particular command, so why bloat the code with extraneous
messages? Similarly with other errors like ENODEV -- if there is only 1
device in question AND the user specified the device then you do not
need to augment with an additional error message.

The value of extack is in explaining EINVAL, EOPNOTSUPP, or the
confusing inter-dependencies in command arguments. It is not a matter of
adding an extack message for every single error path; add messages that
provide value in helping the user.