netdev - Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than one message

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cea2b3c4-c5ae-878a-21c4-57ada02aa2f3@gmail.com>
Date:   Sun, 18 Mar 2018 22:27:00 -0600
From:   David Ahern <dsahern@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     netdev@...r.kernel.org, Alexander Aring <aring@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kubakici@...pl>
Subject: Re: [PATCH RFC 1/2] netlink: extend extack so it can carry more than
 one message

On 3/18/18 12:19 PM, Marcelo Ricardo Leitner wrote:
> On Sun, Mar 18, 2018 at 10:11:20AM -0600, David Ahern wrote:
>> On 3/16/18 1:23 PM, Marcelo Ricardo Leitner wrote:
>>> Currently extack can carry only a single message, which is usually the
>>> error message.
>>>
>>> This imposes a limitation on a more verbose error reporting. For
>>> example, it's not able to carry warning messages together with the error
>>> message, or 2 warning messages.
>>
>>
>> The only means for userspace to separate an error message from info or
>> warnings is the error in nlmsgerr. If it is non-0, any extack message is
>> considered an error else it is a warning.
> 
> I don't see your point here.
> 
> The proposed patch extends what you said to:
> - include warnings on error reports
> - allow more than 1 message
> 
> With the proposed patch, if nlmsgerr is 0 all messages are considered
> as warnings. If it's non-zero, some may be marked as warnings.

It's the 'some' that I was referring to, but ...


>>> +#define NL_SET_ERR_MSG(extack, msg)	NL_SET_MSG(extack, msg)
>>> +#define NL_SET_WARN_MSG(extack, msg)	NL_SET_MSG(extack, KERN_WARNING msg)
>>> +
>>>  #define NL_SET_ERR_MSG_MOD(extack, msg)			\
>>>  	NL_SET_ERR_MSG((extack), KBUILD_MODNAME ": " msg)
>>> +#define NL_SET_WARN_MSG_MOD(extack, msg)		\
>>> +	NL_SET_WARN_MSG((extack), KBUILD_MODNAME ": " msg)
>>> +
>>
>> Adding separate macros for error versus warning is confusing since from
>> an extack perspective a message is a message and there is no uapi to
>> separate them.
> 
> Are you saying the markings at beginning of the messages are not
> possible? If that's the case, we probably can think of something else,
> as I see value in being able to deliver warnings together with errors.

... I did miss the KERN_WARNIN above. That means that warning messages
are prefixed by 0x1 (KERN_SOH) and "4" (warning loglevel). There will be
cases missed by iproute2 as current versions won't catch the 2 new
characters.

Converting code to be able to continue generating error messages yet
ultimately fail seems overly complex to me. I get the intent of
returning as much info as possible, but most of that feels (e.g., in the
mlx5 example you referenced) like someone learning how to do something
the first time in which case 1 at a time errors seems reasonable - in
fact might drive home some lessons. ;-)