[PATCH] scripts: check-sysctl-docs: adapt to new API

[PATCH] 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.

In its reference invocation the script won't be able to parse the tables
from ipc/ipc_sysctl.c as they are using dynamically built tables which
are to complex to parse.

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>
---
 scripts/check-sysctl-docs | 42 ++++++++++++------------------------------
 1 file changed, 12 insertions(+), 30 deletions(-)

diff --git a/scripts/check-sysctl-docs b/scripts/check-sysctl-docs
index 4f163e0bf6a4..bd18ab4b950b 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,12 @@ 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 +92,24 @@ 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 +118,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 {

---
base-commit: 1a44b0073b9235521280e19d963b6dfef7888f18
change-id: 20231220-sysctl-check-8802651d945d

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

Re: [PATCH] scripts: check-sysctl-docs: adapt to new API

[Joel Granados]

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

On Wed, Dec 20, 2023 at 10:30:26PM +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.
> 
> In its reference invocation the script won't be able to parse the tables
> from ipc/ipc_sysctl.c as they are using dynamically built tables which
> are to complex to parse.
> 
> 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>
> ---
>  scripts/check-sysctl-docs | 42 ++++++++++++------------------------------
>  1 file changed, 12 insertions(+), 30 deletions(-)
> 
> diff --git a/scripts/check-sysctl-docs b/scripts/check-sysctl-docs
> index 4f163e0bf6a4..bd18ab4b950b 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,12 @@ 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 +92,24 @@ FNR == NR {
>  
>  # Stage 2: process each file and find all sysctl tables
>  BEGINFILE {
> -    delete children
>      delete entries
>      delete paths
Why did you leave paths? As I read it you remove the use of paths and
now this is not needed any longer.

> -    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)
Would these regular expressions match lines that have more than one
spaces before const?

> +    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 +118,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 {
> 
> ---
> base-commit: 1a44b0073b9235521280e19d963b6dfef7888f18
> 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] scripts: check-sysctl-docs: adapt to new API

[Thomas Weißschuh]

Dec 24, 2023 19:44:42 Joel Granados <j.granados@samsung.com>:

> On Wed, Dec 20, 2023 at 10:30:26PM +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.
>>
>> In its reference invocation the script won't be able to parse the tables
>> from ipc/ipc_sysctl.c as they are using dynamically built tables which
>> are to complex to parse.
>>
>> 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>
>> ---
>> scripts/check-sysctl-docs | 42 ++++++++++++------------------------------
>> 1 file changed, 12 insertions(+), 30 deletions(-)
>>
>> diff --git a/scripts/check-sysctl-docs b/scripts/check-sysctl-docs
>> index 4f163e0bf6a4..bd18ab4b950b 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,12 @@ 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 +92,24 @@ FNR == NR {
>>
>> # Stage 2: process each file and find all sysctl tables
>> BEGINFILE {
>> -    delete children
>>      delete entries
>>      delete paths
> Why did you leave paths? As I read it you remove the use of paths and
> now this is not needed any longer.

Good catch, I'll drop it for V2.

>
>> -    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)
> Would these regular expressions match lines that have more than one
> spaces before const?

No. But it is consistent with the other regexes.

>> +    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 +118,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 {
>>
>> ---
>> base-commit: 1a44b0073b9235521280e19d963b6dfef7888f18
>> change-id: 20231220-sysctl-check-8802651d945d
>>
>> Best regards,
>> --
>> Thomas Weißschuh <linux@weissschuh.net>
>>
>
> --
>
> Joel Granados

Re: [PATCH] scripts: check-sysctl-docs: adapt to new API

[Joel Granados]

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

On Sun, Dec 24, 2023 at 11:38:32PM +0100, Thomas Weißschuh  wrote:
> Dec 24, 2023 19:44:42 Joel Granados <j.granados@samsung.com>:
> 
> > On Wed, Dec 20, 2023 at 10:30:26PM +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.
> >>
> >> In its reference invocation the script won't be able to parse the tables
> >> from ipc/ipc_sysctl.c as they are using dynamically built tables which
> >> are to complex to parse.
> >>
> >> 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>
> >> ---
> >> scripts/check-sysctl-docs | 42 ++++++++++++------------------------------
> >> 1 file changed, 12 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/scripts/check-sysctl-docs b/scripts/check-sysctl-docs
> >> index 4f163e0bf6a4..bd18ab4b950b 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,12 @@ 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 +92,24 @@ FNR == NR {
> >>
> >> # Stage 2: process each file and find all sysctl tables
> >> BEGINFILE {
> >> -    delete children
> >>      delete entries
> >>      delete paths
> > Why did you leave paths? As I read it you remove the use of paths and
> > now this is not needed any longer.
> 
> Good catch, I'll drop it for V2.
> 
> >
> >> -    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)
> > Would these regular expressions match lines that have more than one
> > spaces before const?
> 
> No. But it is consistent with the other regexes.
I would rather see a construct like "[\t ]+" for these cases so we avoid
missing lines that do not have the linux kernel code conventions.

I'm actually seeing some false positives here not related to the space
before "const" but with the way we match. When I run
`./scripts/check-sysctl-docs -vtable="kernel" Documentation/admin-guide/sysctl/kernel.rst $(git grep -l register_sysctl)`
after applying your patch, I get that "msgmni" has no implementation;
but I can see that it is implemented in `vim ipc/ipc_sysctl.c`.
The culprit is that this match does not consider the cases where we use
kmemdup to create the ctl_tables. This is not related to your patch, but
it is something you might consider addressing now that you are here.

Best
> 
> >> +    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 +118,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 {
> >>
> >> ---
> >> base-commit: 1a44b0073b9235521280e19d963b6dfef7888f18
> >> change-id: 20231220-sysctl-check-8802651d945d
> >>
> >> Best regards,
> >> --
> >> Thomas Weißschuh <linux@weissschuh.net>
> >>
> >
> > --
> >
> > Joel Granados
> 

-- 

Joel Granados

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