linux-kernel - Re: [PATCH 3/7] docs: kdoc: clean up the create_parameter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250813011746.6779c0f1@foz.lan>
Date: Wed, 13 Aug 2025 01:17:46 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Jonathan Corbet <corbet@....net>
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, Akira Yokosawa
 <akiyks@...il.com>
Subject: Re: [PATCH 3/7] docs: kdoc: clean up the create_parameter_list()
 "first arg" logic

On Tue, 12 Aug 2025 13:57:44 -0600
Jonathan Corbet <corbet@....net> wrote:

> The logic for finding the name of the first in a series of variable names
> is somewhat convoluted and, in the use of .extend(), actively buggy.
> Document what is happening and simplify the logic.
> 
> Signed-off-by: Jonathan Corbet <corbet@....net>
> ---
>  scripts/lib/kdoc/kdoc_parser.py | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/lib/kdoc/kdoc_parser.py b/scripts/lib/kdoc/kdoc_parser.py
> index 53051ce831ba..47f7ea01ed10 100644
> --- a/scripts/lib/kdoc/kdoc_parser.py
> +++ b/scripts/lib/kdoc/kdoc_parser.py
> @@ -553,18 +553,18 @@ class KernelDoc:
>                  arg = KernRe(r'\s*\[').sub('[', arg)
>                  args = KernRe(r'\s*,\s*').split(arg)
>                  args[0] = re.sub(r'(\*+)\s*', r' \1', args[0])
> -
> -                first_arg = []
> -                r = KernRe(r'^(.*\s+)(.*?\[.*\].*)$')
> -                if args[0] and r.match(args[0]):
> -                    args.pop(0)
> -                    first_arg.extend(r.group(1))
> -                    first_arg.append(r.group(2))

I double-checked the Perl code. The Python version seems to be an exact
translation of what was there:

            $arg =~ s/\s*\[/\[/g;

            my @args = split('\s*,\s*', $arg);
            if ($args[0] =~ m/\*/) {
                $args[0] =~ s/(\*+)\s*/ $1/;
            }

	    my @first_arg;
            if ($args[0] =~ /^(.*\s+)(.*?\[.*\].*)$/) {
                shift @args;
                push(@first_arg, split('\s+', $1));
                push(@first_arg, $2);
            } else {
                @first_arg = split('\s+', shift @args);
            }

Yeah, I agree that this logic is confusing. 

> +                #
> +                # args[0] has a string of "type a".  If "a" includes an [array]
> +                # declaration, we want to not be fooled by any white space inside
> +                # the brackets, so detect and handle that case specially.
> +                #
> +                r = KernRe(r'^([^[\]]*\s+)' r'((?:.*?\[.*\].*)|(?:.*?))$')

Same comment as patch 6/7... concats in the middle of the like IMO makes it
harder to read. Better to place them on separate lines:

	r = KernRe(r'^([^[\]]*\s+)'
		   r'((?:.*?\[.*\].*)|(?:.*?))$')

> +                if r.match(args[0]):
> +                    args[0] = r.group(2)
> +                    dtype = r.group(1)
>                  else:
> -                    first_arg = KernRe(r'\s+').split(args.pop(0))
> -
> -                args.insert(0, first_arg.pop())
> -                dtype = ' '.join(first_arg)
> +                    # No space in args[0]; this seems wrong but preserves previous behavior
> +                    dtype = ''
>  
>                  bitfield_re = KernRe(r'(.*?):(\w+)')
>                  for param in args:

I didn't test your new code. On a first glance, it doesn't seem identical
to the previous one, but if you tested it and the results are the same,
the new version seems nicer once you split the concat on two lines. So,
feel free to add:

Acked-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>


-

Btw, IMHO, it would make sense to have unittests to check things
like that to ensure that new patches won't cause regressions for
some particular usecases.

Thanks,
Mauro