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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <79452f68-c231-4bf2-a4ea-e3dce9b78e2e@kernel.org>
Date: Thu, 4 Sep 2025 18:48:12 +0900
From: Vincent Mailhol <mailhol@...nel.org>
To: Oliver Hartkopp <socketcan@...tkopp.net>,
 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/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.


Yours sincerely,
Vincent Mailhol