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: <20180319153447.GK9345@localhost.localdomain>
Date:   Mon, 19 Mar 2018 12:34:47 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     David Ahern <dsahern@...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 Sun, Mar 18, 2018 at 10:27:00PM -0600, David Ahern wrote:
> 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.

The first one is not printable, so it would print a weird '4' at the
beginning of the message. But: only if it didn't have any error
message later, because old iproute will display only the last message
(and error messages are not tagged).

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

That is true.

Yep, I'm still lacking a real user for it. Maybe with the patchset
split it will come up.

  M.