Re: [PATCH] devtool: fix falsely reporting from checkpatch

[David Marchand <david.marchand@redhat.com>]

Hi,

On Mon, Jan 20, 2025 at 12:27 PM WanRenyong <wanry@yunsilicon.com> wrote:
>
> When executes the check_packed_attributes function in checkpatch,
> if __rte_packed_begin or __rte_packed_end appear in the context
> of a patch file, there may be a situation where the counts of
> __rte_packed_begin and __rte_packed_end do not match, causing
> checkpatch to return a failure.
> This patch fixes this issue by only counting the lines in the
> patch file that start with a + and include either
> __rte_packed_begin or __rte_packed_end.
>
> Signed-off-by: WanRenyong <wanry@yunsilicon.com>

Thanks for proposing this fix.

When sending such fixes, don't forget to add a Fixes: and Cc:
author/maintainers by using --cc-cmd devtools/get-maintainers.sh


Adding Akhil, André and Thomas in the loop.

I also had some concern about the check:
https://inbox.dpdk.org/dev/CAJFAV8w=s1L-WYk+Qv-B+Mn6eAwKrB=GTz6hU--ZoLrJsz7=DQ@mail.gmail.com/

But I merged the check untouched as nobody else seemed to object.
Checkpatch warnings are known to have false positive and such false
positives are "filtered" by subtree maintainers.


> ---
>  devtools/checkpatches.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 003bb49e04..2e228b7f92 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -384,8 +384,8 @@ check_packed_attributes() { # <patch>
>                 res=1
>         fi
>
> -       begin_count=$(grep '__rte_packed_begin' "$1" | wc -l)
> -       end_count=$(grep '__rte_packed_end' "$1" | wc -l)
> +       begin_count=$(grep -E '^\+.*__rte_packed_begin' "$1" | wc -l)
> +       end_count=$(grep -E '^\+.*__rte_packed_end' "$1" | wc -l)
>         if [ $begin_count != $end_count ]; then
>                 echo "__rte_packed_begin and __rte_packed_end should always be used in pairs."
>                 res=1

I don't think this change is any better.

There is a good chance that a patch touching just a first line of a
structure definition won't come along a line touching the last line of
the struct.
My suggestion (if we want to avoid those non useful warning) would be
to just remove the counting stuff in the check.

Opinions?


-- 
David Marchand