Message-ID: <20250617183124.GC2545@horms.kernel.org>
Date: Tue, 17 Jun 2025 19:31:24 +0100
From: Simon Horman <horms@...nel.org>
To: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
Cc: corbet@....net, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux-doc@...r.kernel.org,
	linux-kernel-mentees@...ts.linux.dev, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, skhan@...uxfoundation.com,
	jacob.e.keller@...el.com, alok.a.tiwari@...cle.com
Subject: Re: [PATCH v2 1/2] docs: net: sysctl documentation cleanup

On Sun, Jun 15, 2025 at 01:53:23AM +0300, Abdelrahman Fekry wrote:
> I noticed that some boolean parameters have missing default values
> (enabled/disabled) in the documentation so i checked the initialization
> functions to get their default values, also there was some inconsistency
> in the representation. During the process , i stumbled upon a typo in
> cipso_rbm_struct_valid instead of cipso_rbm_struct_valid.

Please consider using the imperative mood in patch discriptions.

As per [*] please denote the target tree for Networking patches.
In this case net-next seems appropriate.

  [PATCH net-next v3 1/2] ...

[*] https://docs.kernel.org/process/maintainer-netdev.html

And please make sure the patches apply cleanly, without fuzz, on
top of the target tree: this series seems to apply cleanly neither
on net or net-next.

The text below, up to (but not including your Signed-off-by line)
doesn't belong in the patch description. If you wish to include
notes or commentary of this nature then please do so below the
scissors ("---"). But I think the brief summary you already
have there is sufficient in this case - we can follow
the link to v1 for more information.

> 
> Thanks for the review.
> 
> On Thu, 12 Jun 2025, Jacob Keller wrote:
> > Would it make sense to use "0 (disabled)" and "1 (enabled)" with
> > parenthesis for consistency with the default value?
> 
> Used as suggested.
> 
> On Fri, 13 Jun 2025, ALOK TIWARI wrote:
> > for consistency
> > remove extra space before colon
> > Default: 1 (enabled)
> 
> Fixed. 
> 
> On Sat, 14 Jun 2025 10:46:29 -0700, Jakub Kicinski wrote:
> > You need to repost the entire series. Make sure you read:
> > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> > before you do.
> 
> Reposted the entire series, Thanks for you patiency.
> 
> Signed-off-by: Abdelrahman Fekry <abdelrahmanfekry375@...il.com>
> ---
> v2:
> - Deleted space before colon for consistency
> - Standardized more boolean representation (0/1 with enabled/disabled)
> 
> v1: https://lore.kernel.org/all/20250612162954.55843-2-abdelrahmanfekry375@gmail.com/
> - Fixed typo in cipso_rbm_struct_valid
> - Added missing default value declarations
> - Standardized boolean representation (0/1 with enabled/disabled)
>  Documentation/networking/ip-sysctl.rst | 47 ++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/networking/ip-sysctl.rst b/Documentation/networking/ip-sysctl.rst
> index 0f1251cce314..68778532faa5 100644
> --- a/Documentation/networking/ip-sysctl.rst
> +++ b/Documentation/networking/ip-sysctl.rst
> @@ -8,14 +8,16 @@ IP Sysctl
>  ==============================
>  
>  ip_forward - BOOLEAN
> -	- 0 - disabled (default)
> -	- not 0 - enabled
> +	- 0 (disabled)
> +	- not 0 (enabled)
>  
>  	Forward Packets between interfaces.
>  
>  	This variable is special, its change resets all configuration
>  	parameters to their default state (RFC1122 for hosts, RFC1812
>  	for routers)
> +
> +	Default: 0 (disabled)
>  
>  ip_default_ttl - INTEGER
>  	Default value of TTL field (Time To Live) for outgoing (but not
> @@ -75,7 +77,7 @@ fwmark_reflect - BOOLEAN
>  	If unset, these packets have a fwmark of zero. If set, they have the
>  	fwmark of the packet they are replying to.

Maybe it would be more consistent to describe this in terms
of enabled / disabled rather than set / unset.

>  
> -	Default: 0
> +	Default: 0 (disabled)
>  
>  fib_multipath_use_neigh - BOOLEAN
>  	Use status of existing neighbor entry when determining nexthop for
> @@ -368,7 +370,7 @@ tcp_autocorking - BOOLEAN
>  	queue. Applications can still use TCP_CORK for optimal behavior
>  	when they know how/when to uncork their sockets.
>  
> -	Default : 1
> +	Default: 1 (enabled)

For consistency, would it make sense to document the possible values here.

>  
>  tcp_available_congestion_control - STRING
>  	Shows the available congestion control choices that are registered.
> @@ -407,6 +409,12 @@ tcp_congestion_control - STRING
>  
>  tcp_dsack - BOOLEAN
>  	Allows TCP to send "duplicate" SACKs.
> +
> +	Possible values:
> +		- 0 (disabled)
> +		- 1 (enabled)

In the case of ip_forward, the possible values are not explicitly named
as such and appear at the top of the documentation for the parameter.

Here they are explicitly named possible values and appear below the
description of the parameter, but before documentation of the Default.
Elsewhere, e.g. ip_forward_use_pmtu, they appear after the documentation of
the Default. And sometimes, e.g. ip_default_ttl, the possible values are
documented at all.

Likewise, indentation and use of blank lines seems inconsistent.

Is there a value in cleaning this up too?

> +
> +	Default: 1 (enabled)
>  

...

-- 
pw-bot: changes-requested

Powered by blists - more mailing lists