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 10:14:00 -0600
From:   David Ahern <dsahern@...il.com>
To:     Parav Pandit <parav@...dia.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

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#break
>>> 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.