[PATCH v3 0/3] scripts: check-sysctl-docs: adapt to new API

[PATCH v3 0/3] scripts: check-sysctl-docs: adapt to new API

[Thomas Weißschuh]

The script expects the old sysctl_register_paths() API which was removed
some time ago. Adapt it to work with the new
sysctl_register()/sysctl_register_sz()/sysctl_register_init() APIs.

Per-namespace tables via __register_sysctl_table() are also handled.

Note that the script is already prepared for a potential constification
of the ctl_table structs.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v3:
- Handle per-namespace tables
- Link to v2: https://lore.kernel.org/r/20231226-sysctl-check-v2-1-2d4f50b30d34@weissschuh.net

Changes in v2:
- Remove unused global variable "paths"
- Remove docs for deleted variables "children" and "paths"
- Link to v1: https://lore.kernel.org/r/20231220-sysctl-check-v1-1-420ced4a69d7@weissschuh.net

---
Thomas Weißschuh (3):
      scripts: check-sysctl-docs: adapt to new API
      ipc: remove linebreaks from arguments of __register_sysctl_table
      scripts: check-sysctl-docs: handle per-namespace sysctls

 ipc/ipc_sysctl.c          |  3 +--
 scripts/check-sysctl-docs | 65 +++++++++++++++++++++++------------------------
 2 files changed, 33 insertions(+), 35 deletions(-)
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20231220-sysctl-check-8802651d945d

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v3 1/3] scripts: check-sysctl-docs: adapt to new API

[Thomas Weißschuh]

The script expects the old sysctl_register_paths() API which was removed
some time ago. Adapt it to work with the new
sysctl_register()/sysctl_register_sz()/sysctl_register_init() APIs.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 scripts/check-sysctl-docs | 45 ++++++++++++---------------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/scripts/check-sysctl-docs b/scripts/check-sysctl-docs
index 4f163e0bf6a4..739afd766708 100755
--- a/scripts/check-sysctl-docs
+++ b/scripts/check-sysctl-docs
@@ -8,7 +8,7 @@
 # Example invocation:
 #	scripts/check-sysctl-docs -vtable="kernel" \
 #		Documentation/admin-guide/sysctl/kernel.rst \
-#		$(git grep -l register_sysctl_)
+#		$(git grep -l register_sysctl)
 #
 # Specify -vdebug=1 to see debugging information
 
@@ -20,14 +20,10 @@ BEGIN {
 }
 
 # The following globals are used:
-# children: maps ctl_table names and procnames to child ctl_table names
 # documented: maps documented entries (each key is an entry)
 # entries: maps ctl_table names and procnames to counts (so
 #          enumerating the subkeys for a given ctl_table lists its
 #          procnames)
-# files: maps procnames to source file names
-# paths: maps ctl_path names to paths
-# curpath: the name of the current ctl_path struct
 # curtable: the name of the current ctl_table struct
 # curentry: the name of the current proc entry (procname when parsing
 #           a ctl_table, constructed path when parsing a ctl_path)
@@ -94,44 +90,23 @@ FNR == NR {
 
 # Stage 2: process each file and find all sysctl tables
 BEGINFILE {
-    delete children
     delete entries
-    delete paths
-    curpath = ""
     curtable = ""
     curentry = ""
     if (debug) print "Processing file " FILENAME
 }
 
-/^static struct ctl_path/ {
-    match($0, /static struct ctl_path ([^][]+)/, tables)
-    curpath = tables[1]
-    if (debug) print "Processing path " curpath
-}
-
-/^static struct ctl_table/ {
-    match($0, /static struct ctl_table ([^][]+)/, tables)
-    curtable = tables[1]
+/^static( const)? struct ctl_table/ {
+    match($0, /static( const)? struct ctl_table ([^][]+)/, tables)
+    curtable = tables[2]
     if (debug) print "Processing table " curtable
 }
 
 /^};$/ {
-    curpath = ""
     curtable = ""
     curentry = ""
 }
 
-curpath && /\.procname[\t ]*=[\t ]*".+"/ {
-    match($0, /.procname[\t ]*=[\t ]*"([^"]+)"/, names)
-    if (curentry) {
-	curentry = curentry "/" names[1]
-    } else {
-	curentry = names[1]
-    }
-    if (debug) print "Setting path " curpath " to " curentry
-    paths[curpath] = curentry
-}
-
 curtable && /\.procname[\t ]*=[\t ]*".+"/ {
     match($0, /.procname[\t ]*=[\t ]*"([^"]+)"/, names)
     curentry = names[1]
@@ -140,10 +115,14 @@ curtable && /\.procname[\t ]*=[\t ]*".+"/ {
     file[curentry] = FILENAME
 }
 
-/\.child[\t ]*=/ {
-    child = trimpunct($NF)
-    if (debug) print "Linking child " child " to table " curtable " entry " curentry
-    children[curtable][curentry] = child
+/register_sysctl.*/ {
+    match($0, /register_sysctl(|_init|_sz)\("([^"]+)" *, *([^,)]+)/, tables)
+    if (debug) print "Registering table " tables[3] " at " tables[2]
+    if (tables[2] == table) {
+        for (entry in entries[tables[3]]) {
+            printentry(entry)
+        }
+    }
 }
 
 END {

-- 
2.43.2

[PATCH v3 2/3] ipc: remove linebreaks from arguments of __register_sysctl_table

[Thomas Weißschuh]

Calls to __register_sysctl_table will be validated by
scripts/check-sysctl-docs. As this script is line-based remove the
linebreak which would confuse the script.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 ipc/ipc_sysctl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
index 8c62e443f78b..e4008288a3ba 100644
--- a/ipc/ipc_sysctl.c
+++ b/ipc/ipc_sysctl.c
@@ -259,8 +259,7 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
 				tbl[i].data = NULL;
 		}
 
-		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set,
-							  "kernel", tbl,
+		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set, "kernel", tbl,
 							  ARRAY_SIZE(ipc_sysctls));
 	}
 	if (!ns->ipc_sysctls) {

-- 
2.43.2

[PATCH v3 3/3] scripts: check-sysctl-docs: handle per-namespace sysctls

[Thomas Weißschuh]

Some sysctl tables are registered for each namespace.
(Like in ipc/ipc_sysctl.c)
These need special handling to track the variable assignments.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 scripts/check-sysctl-docs | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/scripts/check-sysctl-docs b/scripts/check-sysctl-docs
index 739afd766708..20274c63e745 100755
--- a/scripts/check-sysctl-docs
+++ b/scripts/check-sysctl-docs
@@ -93,6 +93,7 @@ BEGINFILE {
     delete entries
     curtable = ""
     curentry = ""
+    delete vars
     if (debug) print "Processing file " FILENAME
 }
 
@@ -105,6 +106,7 @@ BEGINFILE {
 /^};$/ {
     curtable = ""
     curentry = ""
+    delete vars
 }
 
 curtable && /\.procname[\t ]*=[\t ]*".+"/ {
@@ -125,6 +127,24 @@ curtable && /\.procname[\t ]*=[\t ]*".+"/ {
     }
 }
 
+/kmemdup.*/ {
+    match($0, /([^ \t]+) *= *kmemdup\(([^,]+) *,/, names)
+    if (debug) print "Found variable " names[1] " for table " names[2]
+    if (names[2] in entries) {
+        vars[names[1]] = names[2]
+    }
+}
+
+/__register_sysctl_table.*/ {
+    match($0, /__register_sysctl_table\([^,]+, *"([^"]+)" *, *([^,]+)/, tables)
+    if (debug) print "Registering variable table " tables[2] " at " tables[1]
+    if (tables[1] == table && tables[2] in vars) {
+        for (entry in entries[vars[tables[2]]]) {
+            printentry(entry)
+        }
+    }
+}
+
 END {
     for (entry in documented) {
 	if (!seen[entry]) {

-- 
2.43.2

Re: [PATCH v3 0/3] scripts: check-sysctl-docs: adapt to new API

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

Hey Thomas

Thx for your V3. I'll put it on my sysctl todo list for friday.

Best

On Mon, Feb 19, 2024 at 09:19:21PM +0100, Thomas Weißschuh wrote:
> The script expects the old sysctl_register_paths() API which was removed
> some time ago. Adapt it to work with the new
> sysctl_register()/sysctl_register_sz()/sysctl_register_init() APIs.
> 
> Per-namespace tables via __register_sysctl_table() are also handled.
> 
> Note that the script is already prepared for a potential constification
> of the ctl_table structs.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v3:
> - Handle per-namespace tables
> - Link to v2: https://lore.kernel.org/r/20231226-sysctl-check-v2-1-2d4f50b30d34@weissschuh.net
> 
> Changes in v2:
> - Remove unused global variable "paths"
> - Remove docs for deleted variables "children" and "paths"
> - Link to v1: https://lore.kernel.org/r/20231220-sysctl-check-v1-1-420ced4a69d7@weissschuh.net
> 
> ---
> Thomas Weißschuh (3):
>       scripts: check-sysctl-docs: adapt to new API
>       ipc: remove linebreaks from arguments of __register_sysctl_table
>       scripts: check-sysctl-docs: handle per-namespace sysctls
> 
>  ipc/ipc_sysctl.c          |  3 +--
>  scripts/check-sysctl-docs | 65 +++++++++++++++++++++++------------------------
>  2 files changed, 33 insertions(+), 35 deletions(-)
> ---
> base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
> change-id: 20231220-sysctl-check-8802651d945d
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>
> 

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 2/3] ipc: remove linebreaks from arguments of __register_sysctl_table

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On Mon, Feb 19, 2024 at 09:19:23PM +0100, Thomas Weißschuh wrote:
> Calls to __register_sysctl_table will be validated by
> scripts/check-sysctl-docs. As this script is line-based remove the
> linebreak which would confuse the script.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  ipc/ipc_sysctl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/ipc/ipc_sysctl.c b/ipc/ipc_sysctl.c
> index 8c62e443f78b..e4008288a3ba 100644
> --- a/ipc/ipc_sysctl.c
> +++ b/ipc/ipc_sysctl.c
> @@ -259,8 +259,7 @@ bool setup_ipc_sysctls(struct ipc_namespace *ns)
>  				tbl[i].data = NULL;
>  		}
>  
> -		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set,
> -							  "kernel", tbl,
> +		ns->ipc_sysctls = __register_sysctl_table(&ns->ipc_set, "kernel", tbl,
>  							  ARRAY_SIZE(ipc_sysctls));
>  	}
>  	if (!ns->ipc_sysctls) {
This is very interesting and points to a bigger issue with the
check-sysctl-docs: which is that the AWK record separator is "\n" and it
require that the lines being analyzed follow a strict pattern; even if
that meant having a loooong line (not the case in this patch).

The final solution would be to change the separator to something less
line based to something more C based like RS=";{}". This is not a
blocker to this patchset as it actually fixes a broken script. But is an
interesting observation for whomever wants to continue this work.

Thx Thomas.

Best

-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v3 0/3] scripts: check-sysctl-docs: adapt to new API

[Joel Granados]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

On Mon, Feb 19, 2024 at 09:19:21PM +0100, Thomas Weißschuh wrote:
> The script expects the old sysctl_register_paths() API which was removed
> some time ago. Adapt it to work with the new
> sysctl_register()/sysctl_register_sz()/sysctl_register_init() APIs.
> 
> Per-namespace tables via __register_sysctl_table() are also handled.
> 
> Note that the script is already prepared for a potential constification
> of the ctl_table structs.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v3:
> - Handle per-namespace tables
> - Link to v2: https://lore.kernel.org/r/20231226-sysctl-check-v2-1-2d4f50b30d34@weissschuh.net
> 
> Changes in v2:
> - Remove unused global variable "paths"
> - Remove docs for deleted variables "children" and "paths"
> - Link to v1: https://lore.kernel.org/r/20231220-sysctl-check-v1-1-420ced4a69d7@weissschuh.net
> 
> ---
> Thomas Weißschuh (3):
>       scripts: check-sysctl-docs: adapt to new API
>       ipc: remove linebreaks from arguments of __register_sysctl_table
>       scripts: check-sysctl-docs: handle per-namespace sysctls
> 
>  ipc/ipc_sysctl.c          |  3 +--
>  scripts/check-sysctl-docs | 65 +++++++++++++++++++++++------------------------
>  2 files changed, 33 insertions(+), 35 deletions(-)
> ---
> base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
> change-id: 20231220-sysctl-check-8802651d945d
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>
> 
This is fantastic because we went from a totally broken script to one
that has only some false negatives.

I used the following script to test:
for admin_guide in sunrpc kernel index user net abi vm fs ; do
  ./scripts/check-sysctl-docs -vtable="${admin_guide}" Documentation/admin-guide/sysctl/${admin_guide}.rst $(git grep -l sysctl)
done

Before the patch it gives me 205 "No implementations" out of 205
And after the patch it gave me 42 "No implementations" out of 285

There is still some work to be done, especially for the "fs" part
of this script where there remains some strangeness on how we parse the
rst file.

As this patch has not functional changes, I'll merge it in to
sysctl-next

Thx

Reviewed-by: Joel Granados <j.granados@samsung.com>
-- 

Joel Granados

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]