Re: [PATCH v2 1/2] docs: net: sysctl documentation cleanup
From: Simon Horman <horms@kernel.org>
Date: 2025-06-17 18:31:29
Also in:
linux-doc, linux-kernel-mentees, lkml
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.
quoted hunk ↗ jump to hunk
Thanks for the review. On Thu, 12 Jun 2025, Jacob Keller wrote:quoted
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:quoted
for consistency remove extra space before colon Default: 1 (enabled)Fixed. On Sat, 14 Jun 2025 10:46:29 -0700, Jakub Kicinski wrote:quoted
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 <redacted> --- 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/ (local) - 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.
quoted hunk ↗ jump to hunk
- 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.
quoted hunk ↗ jump to hunk
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