devtool: fix falsely reporting from checkpatch

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>
---
 devtools/checkpatches.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

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?
  

On Tue, Jan 28, 2025 at 04:02:27PM +0100, David Marchand wrote:
> 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

Yes, it was known that false positives could happen, as is the case with many other checks used today. While I was working on the patchset that added this new check it helped me identify some issues with my changes. I feel I benefited from this check at that time, but if people feel that it is causing more harm than good, I am not opposed to having it removed.