From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Ahern Subject: Re: [PATCH net-next 1/6] net: sched: sch_api: handle generic qdisc errors Date: Thu, 7 Dec 2017 10:52:01 -0700 Message-ID: <5b8015b8-4fd2-0b1e-2e78-9b318b529f30@gmail.com> References: <20171206160845.6646-1-aring@mojatatu.com> <20171206160845.6646-2-aring@mojatatu.com> <75db5547-08bb-c3cb-ef7b-f3f73f66709d@gmail.com> <61d9b390-3193-3e17-ea65-30f457647eae@mojatatu.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: xiyou.wangcong@gmail.com, jiri@resnulli.us, netdev@vger.kernel.org, kernel@mojatatu.com To: Jamal Hadi Salim , Alexander Aring , davem@davemloft.net Return-path: Received: from mail-pg0-f67.google.com ([74.125.83.67]:38453 "EHLO mail-pg0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752768AbdLGRwE (ORCPT ); Thu, 7 Dec 2017 12:52:04 -0500 Received: by mail-pg0-f67.google.com with SMTP id f12so4967856pgo.5 for ; Thu, 07 Dec 2017 09:52:03 -0800 (PST) In-Reply-To: <61d9b390-3193-3e17-ea65-30f457647eae@mojatatu.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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.