netdev - RE: [PATCH RESEND iproute2-next] devlink: Add optional controller user input

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]

Date:   Mon, 7 Jun 2021 18:26:58 +0000
From:   Parav Pandit <parav@...dia.com>
To:     David Ahern <dsahern@...il.com>,
        "stephen@...workplumber.org" <stephen@...workplumber.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Jiri Pirko <jiri@...dia.com>
Subject: RE: [PATCH RESEND iproute2-next] devlink: Add optional controller
 user input



> From: David Ahern <dsahern@...il.com>
> Sent: Monday, June 7, 2021 9:44 PM
> 
> On 6/7/21 9:12 AM, Parav Pandit wrote:
> >
> >
> >> From: David Ahern <dsahern@...il.com>
> >> Sent: Monday, June 7, 2021 8:11 PM
> >>
> >> On 6/7/21 5:43 AM, Parav Pandit wrote:
> >>> Hi David,
> >>>
> >>>> From: David Ahern <dsahern@...il.com>
> >>>> Sent: Monday, June 7, 2021 8:31 AM
> >>>>
> >>>> On 6/3/21 5:19 AM, Parav Pandit wrote:
> >>>>> @@ -3795,7 +3806,7 @@ static void cmd_port_help(void)
> >>>>>  	pr_err("       devlink port param set DEV/PORT_INDEX name
> >>>> PARAMETER value VALUE cmode { permanent | driverinit | runtime
> >>>> }\n");
> >>>>>  	pr_err("       devlink port param show [DEV/PORT_INDEX name
> >>>> PARAMETER]\n");
> >>>>>  	pr_err("       devlink port health show [ DEV/PORT_INDEX reporter
> >>>> REPORTER_NAME ]\n");
> >>>>> -	pr_err("       devlink port add DEV/PORT_INDEX flavour FLAVOUR
> >>>> pfnum PFNUM [ sfnum SFNUM ]\n");
> >>>>> +	pr_err("       devlink port add DEV/PORT_INDEX flavour
> FLAVOUR
> >>>> pfnum PFNUM [ sfnum SFNUM ] [ controller CNUM ]\n");
> >>>>>  	pr_err("       devlink port del DEV/PORT_INDEX\n");
> >>>>>  }
> >>>>>
> >>>>> @@ -4324,7 +4335,7 @@ static int __cmd_health_show(struct dl *dl,
> >>>>> bool show_device, bool show_port);
> >>>>>
> >>>>>  static void cmd_port_add_help(void)  {
> >>>>> -	pr_err("       devlink port add { DEV | DEV/PORT_INDEX } flavour
> >>>> FLAVOUR pfnum PFNUM [ sfnum SFNUM ]\n");
> >>>>> +	pr_err("       devlink port add { DEV | DEV/PORT_INDEX }
> flavour
> >>>> FLAVOUR pfnum PFNUM [ sfnum SFNUM ] [ controller CNUM ]\n");
> >>>>
> >>>> This line and the one above need to be wrapped. This addition puts
> >>>> it well into the 90s.
> >>>>
> >>> It’s a print message.
> >>> I was following coding style of [1] that says "However, never break
> >>> user-
> >> visible strings such as printk messages because that breaks the
> >> ability to grep for them.".
> >>> Recent code of dcb_ets.c has similar long string in print. So I didn't wrap
> it.
> >>
> >> I missed that when reviewing the dcb command then.
> >>
> >>> Should we warp it?
> >>>
> >>> [1]
> >>> https://www.kernel.org/doc/html/latest/process/coding-style.html#bre
> >>> ak
> >>> ing-long-lines-and-strings
> >>>
> >>
> >> [1] is referring to messages from kernel code, and I agree with that
> >> style. This is help message from iproute2. I tend to keep my terminal
> >> widths between
> >> 80 and 90 columns, so the long help lines from commands are not very
> >> friendly causing me to resize the terminal.
> > I see. So do you recommend splitting the print message?
> > I personally feel easier to follow kernel coding standard as much
> > possible in spirit of "grep them". 😊
> > But its really up to you. Please let me know.
> >
> 
> 
> There are different type of strings:
> 1. help,
> 2. error messages,
> 3. informational messages,
> 4. displaying a configuration
> 
> 1. is "how do I use this command". There is no reason to make that 1 gigantic
> line. All of the iproute2 commands wrap the help. My comment above is on
> this category.
> 
> 2. and 3. should not be wrapped to allow someone to attempt to go from
> "why did I get this output" to a line of code (or many lines). This is the kernel
> reference above.
> 
> 4. Displaying attributes and settings for some object getting dumped.
> The lines can get really long and unreadable to humans; these should be split
> across multiple lines - like iproute2 commands do. There is no reason for this
> to be on online unless the user asks for it via -oneline option.

Thanks David for the detailed explanation.
It totally make sense. I am fixing it.