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

[Joel Granados]

On Sun, Dec 24, 2023 at 11:38:32PM +0100, Thomas Weißschuh  wrote:
> Dec 24, 2023 19:44:42 Joel Granados <j.granados@xxxxxxxxxxx>:
> 
> > 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@xxxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxxxx>
> >>
> >
> > --
> >
> > Joel Granados
> 

-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature