linux-kernel - Re: [PATCH 07/21] can: netlink: remove comment in can

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a4c0fade-52b6-4077-8a6a-fce6f2d62cd2@hartkopp.net>
Date: Fri, 5 Sep 2025 12:55:01 +0200
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Vincent Mailhol <mailhol@...nel.org>,
 Marc Kleine-Budde <mkl@...gutronix.de>
Cc: Stéphane Grosjean <stephane.grosjean@...-networks.com>,
 Robert Nawrath <mbro1689@...il.com>, Minh Le <minh.le.aj@...esas.com>,
 Duy Nguyen <duy.nguyen.rh@...esas.com>, linux-can@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/21] can: netlink: remove comment in can_validate()



On 04.09.25 11:48, Vincent Mailhol wrote:
> On 04/09/2025 at 15:51, Oliver Hartkopp wrote:
>> Hi Vincent,
>>
>> On 03.09.25 10:50, Vincent Mailhol wrote:
>>> The comment in can_validate() is just paraphrasing the code. When
>>> adding CAN XL, updating this comment would add some overhead work for
>>> no clear benefit.
>>
>> I generally see that the code introduced by yourself has nearly no comments.
> 
> I tend to disagree. While it is true that I added no C-style comment blocks, I
> added a ton of error messages which I would argue are documentation.
> 
> For example, this code:
> 
> 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
> 	 * must be set and vice-versa
> 	 */
> 	if ((tdc_auto || tdc_manual) != !!data_tdc)
> 		return -EOPNOTSUPP;
> 
> was transformed into:
> 
> 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
> 	 * must be set and vice-versa
> 	 */
> 	if ((tdc_auto || tdc_manual) && !data_tdc) {
> 		NL_SET_ERR_MSG(extack, "TDC parameters are missing");
> 		return -EOPNOTSUPP;
> 	}
> 	if (!(tdc_auto || tdc_manual) && data_tdc) {
> 		NL_SET_ERR_MSG(extack, "TDC mode (auto or manual) is missing");
> 		return -EOPNOTSUPP;
> 	}
> 
> Which I think is a huge improvement on the documentation. And this has real
> value because the user do not have to look at the source code anymore to
> understand why an
> 
>    ip link set can ...
> 
> returned an error.
> 
>> E.g. if you look at the [PATCH 12/21] can: netlink: add
>> can_ctrlmode_changelink() the comments introduced by myself document the
>> different steps as we had problems with the complexity there and it was hard to
>> review either.
> 
> Those comments are still here.
> 
>> I would like to motivate you to generally add more comments.
> This is a refactoring series. I kept all existing comments except of one and
> then added a more comments in the form of error message. I am not adding code,
> just moving it around, so I do not really get why I should be adding even more
> comments to the existing code.
> 
>> When people (like me) look into that code that they haven't written themselves
>> and there is not even a hint of "what's the idea of what we are doing here" then
>> the code is hard to follow and to review.
> 
> Is this an issue in my code refactoring or an issue with the existing code?
> 
>> We definitely don't need a full blown documentation on top of each function. But
>> I like this comment you want to remove here and I would like to have more of it,
>> so that people get an impression what they will see in the following code.
> 

No need to defend yourself with specific references or even feel 
personally attacked.

My overall feeling is that you spend an excellent effort in commit 
messages but this information is then omitted in code comments.

As I've already written "I would like to motivate you to generally add 
more comments.". And this can also happen when refactoring things where 
new functions are created which reduces the context to the original code 
section.

Best regards,
Oliver