[dpdk-dev] [PATCH] devtools: fix check symbol change script

[dpdk-dev] [PATCH] devtools: fix check symbol change script

[Nithin Dabilpuram]

Fix check symbol change script to detect new diff file when
it is in between  "--- /dev/null" to "b/lib/...".
Current awk line expects line to start with "a/..."
which is not always true for all diffs.
As a result if in_map was '1' earlier, it will not be changed
to '0' and we get check patch errors which are not true as the non
version.map files get interpreted as version map file.

Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
Cc: nhorman@tuxdriver.com
Cc: stable@dpdk.org

Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
---
Note: We have two examples where checkpatch errors are because of this
because the version.map file change comes earlier in the diff. Because of 
this bug, any new file change that comes after version.map file diff
as "/dev/null" to "b/.." gets misdetected as version.map file.
* http://patches.dpdk.org/patch/66878/
* https://patchwork.dpdk.org/patch/66900/
 devtools/check-symbol-change.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index c5434f3..19ce82f 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -17,13 +17,13 @@ build_map_changes()
 		# map files are altered, and all section/symbol names
 		# appearing between a triggering of this rule and the
 		# next trigger of this rule are associated with this file
-		/[-+] a\/.*\.map/ {map=$2; in_map=1}
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
 
 		# Same pattern as above, only it matches on anything that
 		# does not end in 'map', indicating we have left the map chunk.
 		# When we hit this, turn off the in_map variable, which
 		# supresses the subordonate rules below
-		/[-+] a\/.*\.[^map]/ {in_map=0}
+		/[-+] [ab]\/.*\.[^map]/ {in_map=0}
 
 		# Triggering this rule, which starts a line and ends it
 		# with a { identifies a versioned section.  The section name is
-- 
2.8.4

Re: [dpdk-dev] [PATCH] devtools: fix check symbol change script

[David Marchand]

On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
<ndabilpuram@marvell.com> wrote:
>
> Fix check symbol change script to detect new diff file when
> it is in between  "--- /dev/null" to "b/lib/...".
> Current awk line expects line to start with "a/..."
> which is not always true for all diffs.
> As a result if in_map was '1' earlier, it will not be changed
> to '0' and we get check patch errors which are not true as the non
> version.map files get interpreted as version map file.
>
> Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
> Cc: nhorman@tuxdriver.com
> Cc: stable@dpdk.org

There was a previous attempt at fixing this that did not get a review.
http://patchwork.dpdk.org/patch/56303/

I prefer the last submitted patch as it is simpler, but maybe I missed
something in Bing patch.

Neil, wdyt?


-- 
David Marchand


>
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> ---
> Note: We have two examples where checkpatch errors are because of this
> because the version.map file change comes earlier in the diff. Because of
> this bug, any new file change that comes after version.map file diff
> as "/dev/null" to "b/.." gets misdetected as version.map file.
> * http://patches.dpdk.org/patch/66878/
> * https://patchwork.dpdk.org/patch/66900/
>  devtools/check-symbol-change.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> index c5434f3..19ce82f 100755
> --- a/devtools/check-symbol-change.sh
> +++ b/devtools/check-symbol-change.sh
> @@ -17,13 +17,13 @@ build_map_changes()
>                 # map files are altered, and all section/symbol names
>                 # appearing between a triggering of this rule and the
>                 # next trigger of this rule are associated with this file
> -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
>
>                 # Same pattern as above, only it matches on anything that
>                 # does not end in 'map', indicating we have left the map chunk.
>                 # When we hit this, turn off the in_map variable, which
>                 # supresses the subordonate rules below
> -               /[-+] a\/.*\.[^map]/ {in_map=0}
> +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
>
>                 # Triggering this rule, which starts a line and ends it
>                 # with a { identifies a versioned section.  The section name is
> --
> 2.8.4
>

Re: [dpdk-dev] [PATCH] devtools: fix check symbol change script

[Neil Horman]

On Thu, Mar 19, 2020 at 03:56:03PM +0100, David Marchand wrote:
> On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
> <ndabilpuram@marvell.com> wrote:
> >
> > Fix check symbol change script to detect new diff file when
> > it is in between  "--- /dev/null" to "b/lib/...".
> > Current awk line expects line to start with "a/..."
> > which is not always true for all diffs.
> > As a result if in_map was '1' earlier, it will not be changed
> > to '0' and we get check patch errors which are not true as the non
> > version.map files get interpreted as version map file.
> >
> > Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
> > Cc: nhorman@tuxdriver.com
> > Cc: stable@dpdk.org
> 
> There was a previous attempt at fixing this that did not get a review.
> http://patchwork.dpdk.org/patch/56303/
> 
> I prefer the last submitted patch as it is simpler, but maybe I missed
> something in Bing patch.
> 
> Neil, wdyt?
> 
I'm not sure why I didn't review the previous patch you refrenced, apologies for
that.

That said, I'm not sure how this patch detects /dev/null patterns. It looks like
you still expect all lines to start with [+-] [ab]...

Neil

> 
> -- 
> David Marchand
> 
> 
> >
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > ---
> > Note: We have two examples where checkpatch errors are because of this
> > because the version.map file change comes earlier in the diff. Because of
> > this bug, any new file change that comes after version.map file diff
> > as "/dev/null" to "b/.." gets misdetected as version.map file.
> > * http://patches.dpdk.org/patch/66878/
> > * https://patchwork.dpdk.org/patch/66900/
> >  devtools/check-symbol-change.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > index c5434f3..19ce82f 100755
> > --- a/devtools/check-symbol-change.sh
> > +++ b/devtools/check-symbol-change.sh
> > @@ -17,13 +17,13 @@ build_map_changes()
> >                 # map files are altered, and all section/symbol names
> >                 # appearing between a triggering of this rule and the
> >                 # next trigger of this rule are associated with this file
> > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
> >
> >                 # Same pattern as above, only it matches on anything that
> >                 # does not end in 'map', indicating we have left the map chunk.
> >                 # When we hit this, turn off the in_map variable, which
> >                 # supresses the subordonate rules below
> > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
> >
> >                 # Triggering this rule, which starts a line and ends it
> >                 # with a { identifies a versioned section.  The section name is
> > --
> > 2.8.4
> >
> 
> 

Re: [dpdk-dev] [EXT] Re: [PATCH] devtools: fix check symbol change script

[Nithin Dabilpuram]

On Thu, Mar 19, 2020 at 11:40:39AM -0400, Neil Horman wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Mar 19, 2020 at 03:56:03PM +0100, David Marchand wrote:
> > On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
> > <ndabilpuram@marvell.com> wrote:
> > >
> > > Fix check symbol change script to detect new diff file when
> > > it is in between  "--- /dev/null" to "b/lib/...".
> > > Current awk line expects line to start with "a/..."
> > > which is not always true for all diffs.
> > > As a result if in_map was '1' earlier, it will not be changed
> > > to '0' and we get check patch errors which are not true as the non
> > > version.map files get interpreted as version map file.
> > >
> > > Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
> > > Cc: nhorman@tuxdriver.com
> > > Cc: stable@dpdk.org
> > 
> > There was a previous attempt at fixing this that did not get a review.
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.dpdk.org_patch_56303_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=HnuykZkNgy_2cx1i0_QuR-C_vdVYLTJ9rSUA1dCM7WI&s=TW-ZZ_dYy8x6EcvJHtYrv9vaIHSmd0GshvClMwiWPi4&e= 
> > 
> > I prefer the last submitted patch as it is simpler, but maybe I missed
> > something in Bing patch.
> > 
> > Neil, wdyt?
> > 
> I'm not sure why I didn't review the previous patch you refrenced, apologies for
> that.
> 
> That said, I'm not sure how this patch detects /dev/null patterns. It looks like
> you still expect all lines to start with [+-] [ab]...

It doesn't detect /dev/null but the line following /dev/null will start with
"b/...". So this checks for .map in that line as well.

> 
> Neil
> 
> > 
> > -- 
> > David Marchand
> > 
> > 
> > >
> > > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > ---
> > > Note: We have two examples where checkpatch errors are because of this
> > > because the version.map file change comes earlier in the diff. Because of
> > > this bug, any new file change that comes after version.map file diff
> > > as "/dev/null" to "b/.." gets misdetected as version.map file.
> > > * https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.org_patch_66878_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=HnuykZkNgy_2cx1i0_QuR-C_vdVYLTJ9rSUA1dCM7WI&s=0szl1YpYLwfb1OF-R5H4_ci3f5XjBUHFrHVp-LdROFI&e= 
> > > * https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.dpdk.org_patch_66900_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=HnuykZkNgy_2cx1i0_QuR-C_vdVYLTJ9rSUA1dCM7WI&s=ugH7B1XKoFQEEopU0sJgqKGwjDJS9Kl9OsijSazmyRc&e= 
> > >  devtools/check-symbol-change.sh | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > > index c5434f3..19ce82f 100755
> > > --- a/devtools/check-symbol-change.sh
> > > +++ b/devtools/check-symbol-change.sh
> > > @@ -17,13 +17,13 @@ build_map_changes()
> > >                 # map files are altered, and all section/symbol names
> > >                 # appearing between a triggering of this rule and the
> > >                 # next trigger of this rule are associated with this file
> > > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
> > >
> > >                 # Same pattern as above, only it matches on anything that
> > >                 # does not end in 'map', indicating we have left the map chunk.
> > >                 # When we hit this, turn off the in_map variable, which
> > >                 # supresses the subordonate rules below
> > > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > > +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
> > >
> > >                 # Triggering this rule, which starts a line and ends it
> > >                 # with a { identifies a versioned section.  The section name is
> > > --
> > > 2.8.4
> > >
> > 
> > 

Re: [dpdk-dev] [EXT] Re: [PATCH] devtools: fix check symbol change script

[Neil Horman]

On Thu, Mar 19, 2020 at 09:15:50PM +0530, Nithin Dabilpuram wrote:
> On Thu, Mar 19, 2020 at 11:40:39AM -0400, Neil Horman wrote:
> > External Email
> > 
> > ----------------------------------------------------------------------
> > On Thu, Mar 19, 2020 at 03:56:03PM +0100, David Marchand wrote:
> > > On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
> > > <ndabilpuram@marvell.com> wrote:
> > > >
> > > > Fix check symbol change script to detect new diff file when
> > > > it is in between  "--- /dev/null" to "b/lib/...".
> > > > Current awk line expects line to start with "a/..."
> > > > which is not always true for all diffs.
> > > > As a result if in_map was '1' earlier, it will not be changed
> > > > to '0' and we get check patch errors which are not true as the non
> > > > version.map files get interpreted as version map file.
> > > >
> > > > Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
> > > > Cc: nhorman@tuxdriver.com
> > > > Cc: stable@dpdk.org
> > > 
> > > There was a previous attempt at fixing this that did not get a review.
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.dpdk.org_patch_56303_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=HnuykZkNgy_2cx1i0_QuR-C_vdVYLTJ9rSUA1dCM7WI&s=TW-ZZ_dYy8x6EcvJHtYrv9vaIHSmd0GshvClMwiWPi4&e= 
> > > 
> > > I prefer the last submitted patch as it is simpler, but maybe I missed
> > > something in Bing patch.
> > > 
> > > Neil, wdyt?
> > > 
> > I'm not sure why I didn't review the previous patch you refrenced, apologies for
> > that.
> > 
> > That said, I'm not sure how this patch detects /dev/null patterns. It looks like
> > you still expect all lines to start with [+-] [ab]...
> 
> It doesn't detect /dev/null but the line following /dev/null will start with
> "b/...". So this checks for .map in that line as well.
> 
Ah, in that case, I'd really be fine with either fix.

Acked-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> > Neil
> > 
> > > 
> > > -- 
> > > David Marchand
> > > 
> > > 
> > > >
> > > > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > > ---
> > > > Note: We have two examples where checkpatch errors are because of this
> > > > because the version.map file change comes earlier in the diff. Because of
> > > > this bug, any new file change that comes after version.map file diff
> > > > as "/dev/null" to "b/.." gets misdetected as version.map file.
> > > > * https://urldefense.proofpoint.com/v2/url?u=http-3A__patches.dpdk.org_patch_66878_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=HnuykZkNgy_2cx1i0_QuR-C_vdVYLTJ9rSUA1dCM7WI&s=0szl1YpYLwfb1OF-R5H4_ci3f5XjBUHFrHVp-LdROFI&e= 
> > > > * https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.dpdk.org_patch_66900_&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=FZ_tPCbgFOh18zwRPO9H0yDx8VW38vuapifdDfc8SFQ&m=HnuykZkNgy_2cx1i0_QuR-C_vdVYLTJ9rSUA1dCM7WI&s=ugH7B1XKoFQEEopU0sJgqKGwjDJS9Kl9OsijSazmyRc&e= 
> > > >  devtools/check-symbol-change.sh | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > > > index c5434f3..19ce82f 100755
> > > > --- a/devtools/check-symbol-change.sh
> > > > +++ b/devtools/check-symbol-change.sh
> > > > @@ -17,13 +17,13 @@ build_map_changes()
> > > >                 # map files are altered, and all section/symbol names
> > > >                 # appearing between a triggering of this rule and the
> > > >                 # next trigger of this rule are associated with this file
> > > > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > > > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
> > > >
> > > >                 # Same pattern as above, only it matches on anything that
> > > >                 # does not end in 'map', indicating we have left the map chunk.
> > > >                 # When we hit this, turn off the in_map variable, which
> > > >                 # supresses the subordonate rules below
> > > > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > > > +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
> > > >
> > > >                 # Triggering this rule, which starts a line and ends it
> > > >                 # with a { identifies a versioned section.  The section name is
> > > > --
> > > > 2.8.4
> > > >
> > > 
> > > 
> 

Re: [dpdk-dev] [PATCH] devtools: fix check symbol change script

[Bing Zhao]

> -----Original Message-----
> From: Neil Horman <nhorman@tuxdriver.com>
> Sent: Thursday, March 19, 2020 11:41 PM
> To: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev <dev@dpdk.org>;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Nithin Dabilpuram
> <ndabilpuram@marvell.com>; dpdk stable <stable@dpdk.org>; Bing
> Zhao <bingz@mellanox.com>
> Subject: Re: [PATCH] devtools: fix check symbol change script
> 
> On Thu, Mar 19, 2020 at 03:56:03PM +0100, David Marchand wrote:
> > On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
> > <ndabilpuram@marvell.com> wrote:
> > >
> > > Fix check symbol change script to detect new diff file when it is in
> > > between  "--- /dev/null" to "b/lib/...".
> > > Current awk line expects line to start with "a/..."
> > > which is not always true for all diffs.
> > > As a result if in_map was '1' earlier, it will not be changed to '0'
> > > and we get check patch errors which are not true as the non
> > > version.map files get interpreted as version map file.
> > >
> > > Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol
> addition")
> > > Cc: nhorman@tuxdriver.com
> > > Cc: stable@dpdk.org
> >
> > There was a previous attempt at fixing this that did not get a review.
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> patch
> >
> work.dpdk.org%2Fpatch%2F56303%2F&amp;data=02%7C01%7Cbingz
> %40mellanox.c
> >
> om%7C19a838a81f4f4649f40f08d7cc1be78a%7Ca652971c7d2e4d9ba6
> a4d149256f46
> >
> 1b%7C0%7C0%7C637202292556475544&amp;sdata=wgz8LssuucgrWlY
> xsf978%2ByRVg
> > 83btxZIm9aD56nekY%3D&amp;reserved=0
> >
> > I prefer the last submitted patch as it is simpler, but maybe I missed
> > something in Bing patch.
> >
> > Neil, wdyt?
> >
> I'm not sure why I didn't review the previous patch you refrenced,
> apologies for that.
> 
> That said, I'm not sure how this patch detects /dev/null patterns. It
> looks like you still expect all lines to start with [+-] [ab]...
> 
> Neil

Hi David & Neil,
The cause and the fixing are almost the same for the two patches.
Nithin's is simpler.
When matching on "/dev/null", it just needs to continue.

While in my patch, I think I also fix the [^map] matching even though it works.
The behavior of RE in awk does not work as expected, .c and .h files don't match on the
"NOT map" at the end of the line, but match on the signle letter not among the "m/a/p".
(Correct me if anything wrong). 

Thanks
Bing

> 
> >
> > --
> > David Marchand
> >
> >
> > >
> > > Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> > > ---
> > > Note: We have two examples where checkpatch errors are because
> of
> > > this because the version.map file change comes earlier in the diff.
> > > Because of this bug, any new file change that comes after
> > > version.map file diff as "/dev/null" to "b/.." gets misdetected as
> version.map file.
> > > *
> > >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2F
> pat
> > >
> ches.dpdk.org%2Fpatch%2F66878%2F&amp;data=02%7C01%7Cbingz%
> 40mellanox
> > > .com%7C19a838a81f4f4649f40f08d7cc1be78a%7Ca652971c7d2e4d
> 9ba6a4d14925
> > >
> 6f461b%7C0%7C0%7C637202292556475544&amp;sdata=ElLbgB9IJ7B6
> kuNTAjKAFr
> > > WpNy8Jdq5%2BmfoRTxN72tI%3D&amp;reserved=0
> > > *
> > >
> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> pa
> > >
> tchwork.dpdk.org%2Fpatch%2F66900%2F&amp;data=02%7C01%7Cbin
> gz%40mella
> > >
> nox.com%7C19a838a81f4f4649f40f08d7cc1be78a%7Ca652971c7d2e4d
> 9ba6a4d14
> > >
> 9256f461b%7C0%7C0%7C637202292556475544&amp;sdata=iT%2Ft9os
> A4HFTQ2kh3
> > > baWClvD3B%2FFdGzIrQgTvB5SfqU%3D&amp;reserved=0
> > >  devtools/check-symbol-change.sh | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/devtools/check-symbol-change.sh
> > > b/devtools/check-symbol-change.sh index c5434f3..19ce82f 100755
> > > --- a/devtools/check-symbol-change.sh
> > > +++ b/devtools/check-symbol-change.sh
> > > @@ -17,13 +17,13 @@ build_map_changes()
> > >                 # map files are altered, and all section/symbol names
> > >                 # appearing between a triggering of this rule and the
> > >                 # next trigger of this rule are associated with this file
> > > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
> > >
> > >                 # Same pattern as above, only it matches on anything that
> > >                 # does not end in 'map', indicating we have left the map
> chunk.
> > >                 # When we hit this, turn off the in_map variable, which
> > >                 # supresses the subordonate rules below
> > > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > > +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
> > >
> > >                 # Triggering this rule, which starts a line and ends it
> > >                 # with a { identifies a versioned section.  The
> > > section name is
> > > --
> > > 2.8.4
> > >
> >
> >

Re: [dpdk-dev] [PATCH] devtools: fix check symbol change script

[Jerin Jacob]

On Thu, Mar 19, 2020 at 8:14 PM Nithin Dabilpuram
<ndabilpuram@marvell.com> wrote:
>
> Fix check symbol change script to detect new diff file when
> it is in between  "--- /dev/null" to "b/lib/...".
> Current awk line expects line to start with "a/..."
> which is not always true for all diffs.
> As a result if in_map was '1' earlier, it will not be changed
> to '0' and we get check patch errors which are not true as the non
> version.map files get interpreted as version map file.
>
> Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
> Cc: nhorman@tuxdriver.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>

Tested-by: Jerin Jacob <jerinj@marvell.com>


> ---
> Note: We have two examples where checkpatch errors are because of this
> because the version.map file change comes earlier in the diff. Because of
> this bug, any new file change that comes after version.map file diff
> as "/dev/null" to "b/.." gets misdetected as version.map file.
> * http://patches.dpdk.org/patch/66878/
> * https://patchwork.dpdk.org/patch/66900/
>  devtools/check-symbol-change.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> index c5434f3..19ce82f 100755
> --- a/devtools/check-symbol-change.sh
> +++ b/devtools/check-symbol-change.sh
> @@ -17,13 +17,13 @@ build_map_changes()
>                 # map files are altered, and all section/symbol names
>                 # appearing between a triggering of this rule and the
>                 # next trigger of this rule are associated with this file
> -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
>
>                 # Same pattern as above, only it matches on anything that
>                 # does not end in 'map', indicating we have left the map chunk.
>                 # When we hit this, turn off the in_map variable, which
>                 # supresses the subordonate rules below
> -               /[-+] a\/.*\.[^map]/ {in_map=0}
> +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
>
>                 # Triggering this rule, which starts a line and ends it
>                 # with a { identifies a versioned section.  The section name is
> --
> 2.8.4
>

Re: [dpdk-dev] [PATCH] devtools: fix check symbol change script

[David Marchand]

On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
<ndabilpuram@marvell.com> wrote:
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> index c5434f3..19ce82f 100755
> --- a/devtools/check-symbol-change.sh
> +++ b/devtools/check-symbol-change.sh
> @@ -17,13 +17,13 @@ build_map_changes()
>                 # map files are altered, and all section/symbol names
>                 # appearing between a triggering of this rule and the
>                 # next trigger of this rule are associated with this file
> -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
>
>                 # Same pattern as above, only it matches on anything that
>                 # does not end in 'map', indicating we have left the map chunk.
>                 # When we hit this, turn off the in_map variable, which
>                 # supresses the subordonate rules below
> -               /[-+] a\/.*\.[^map]/ {in_map=0}
> +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}

[^map] does not mean "not word map" but actually "none of the
character m, a or p".

So potentially, any file extension starting with m, a or p could
trigger an issue.
For example, a change in .mk files, .py files and
./devtools/check-forbidden-tokens.awk ./devtools/libabigail.abignore
could lead to incorrect symbol check.

How about the following change:

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index c5434f3bb0..ed2178e36e 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -17,13 +17,11 @@ build_map_changes()
                # map files are altered, and all section/symbol names
                # appearing between a triggering of this rule and the
                # next trigger of this rule are associated with this file
-               /[-+] a\/.*\.map/ {map=$2; in_map=1}
+               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}

-               # Same pattern as above, only it matches on anything that
-               # does not end in 'map', indicating we have left the map chunk.
-               # When we hit this, turn off the in_map variable, which
-               # supresses the subordonate rules below
-               /[-+] a\/.*\.[^map]/ {in_map=0}
+               # The previous rule catches all .map files, anything else
+               # indicates we left the map chunk.
+               /[-+] [ab]\// {in_map=0}

                # Triggering this rule, which starts a line and ends it
                # with a { identifies a versioned section.  The section name is


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH] devtools: fix check symbol change script

[Nithin Dabilpuram]

On Mon, Mar 23, 2020 at 09:13:22AM +0100, David Marchand wrote:
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
> <ndabilpuram@marvell.com> wrote:
> > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > index c5434f3..19ce82f 100755
> > --- a/devtools/check-symbol-change.sh
> > +++ b/devtools/check-symbol-change.sh
> > @@ -17,13 +17,13 @@ build_map_changes()
> >                 # map files are altered, and all section/symbol names
> >                 # appearing between a triggering of this rule and the
> >                 # next trigger of this rule are associated with this file
> > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
> >
> >                 # Same pattern as above, only it matches on anything that
> >                 # does not end in 'map', indicating we have left the map chunk.
> >                 # When we hit this, turn off the in_map variable, which
> >                 # supresses the subordonate rules below
> > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
> 
> [^map] does not mean "not word map" but actually "none of the
> character m, a or p".
> 
> So potentially, any file extension starting with m, a or p could
> trigger an issue.
> For example, a change in .mk files, .py files and
> ./devtools/check-forbidden-tokens.awk ./devtools/libabigail.abignore
> could lead to incorrect symbol check.
> 
> How about the following change:
> 
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> index c5434f3bb0..ed2178e36e 100755
> --- a/devtools/check-symbol-change.sh
> +++ b/devtools/check-symbol-change.sh
> @@ -17,13 +17,11 @@ build_map_changes()
>                 # map files are altered, and all section/symbol names
>                 # appearing between a triggering of this rule and the
>                 # next trigger of this rule are associated with this file
> -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
> 
> -               # Same pattern as above, only it matches on anything that
> -               # does not end in 'map', indicating we have left the map chunk.
> -               # When we hit this, turn off the in_map variable, which
> -               # supresses the subordonate rules below
> -               /[-+] a\/.*\.[^map]/ {in_map=0}
> +               # The previous rule catches all .map files, anything else
> +               # indicates we left the map chunk.
> +               /[-+] [ab]\// {in_map=0}
> 
>                 # Triggering this rule, which starts a line and ends it
>                 # with a { identifies a versioned section.  The section name is
> 
> 

Agreed. I tested and it works fine for me. I can send a V2 with this change if it is fine.
> -- 
> David Marchand
> 

Re: [dpdk-dev] [dpdk-stable] [EXT] Re: [PATCH] devtools: fix check symbol change script

[David Marchand]

On Mon, Mar 23, 2020 at 10:28 AM Nithin Dabilpuram
<ndabilpuram@marvell.com> wrote:
> On Mon, Mar 23, 2020 at 09:13:22AM +0100, David Marchand wrote:
> > ----------------------------------------------------------------------
> > On Thu, Mar 19, 2020 at 3:44 PM Nithin Dabilpuram
> > <ndabilpuram@marvell.com> wrote:
> > > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > > index c5434f3..19ce82f 100755
> > > --- a/devtools/check-symbol-change.sh
> > > +++ b/devtools/check-symbol-change.sh
> > > @@ -17,13 +17,13 @@ build_map_changes()
> > >                 # map files are altered, and all section/symbol names
> > >                 # appearing between a triggering of this rule and the
> > >                 # next trigger of this rule are associated with this file
> > > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1}
> > >
> > >                 # Same pattern as above, only it matches on anything that
> > >                 # does not end in 'map', indicating we have left the map chunk.
> > >                 # When we hit this, turn off the in_map variable, which
> > >                 # supresses the subordonate rules below
> > > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > > +               /[-+] [ab]\/.*\.[^map]/ {in_map=0}
> >
> > [^map] does not mean "not word map" but actually "none of the
> > character m, a or p".
> >
> > So potentially, any file extension starting with m, a or p could
> > trigger an issue.
> > For example, a change in .mk files, .py files and
> > ./devtools/check-forbidden-tokens.awk ./devtools/libabigail.abignore
> > could lead to incorrect symbol check.
> >
> > How about the following change:
> >
> > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > index c5434f3bb0..ed2178e36e 100755
> > --- a/devtools/check-symbol-change.sh
> > +++ b/devtools/check-symbol-change.sh
> > @@ -17,13 +17,11 @@ build_map_changes()
> >                 # map files are altered, and all section/symbol names
> >                 # appearing between a triggering of this rule and the
> >                 # next trigger of this rule are associated with this file
> > -               /[-+] a\/.*\.map/ {map=$2; in_map=1}
> > +               /[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
> >
> > -               # Same pattern as above, only it matches on anything that
> > -               # does not end in 'map', indicating we have left the map chunk.
> > -               # When we hit this, turn off the in_map variable, which
> > -               # supresses the subordonate rules below
> > -               /[-+] a\/.*\.[^map]/ {in_map=0}
> > +               # The previous rule catches all .map files, anything else
> > +               # indicates we left the map chunk.
> > +               /[-+] [ab]\// {in_map=0}
> >
> >                 # Triggering this rule, which starts a line and ends it
> >                 # with a { identifies a versioned section.  The section name is
> >
> >
>
> Agreed. I tested and it works fine for me. I can send a V2 with this change if it is fine.

Sure, thanks!


-- 
David Marchand

[dpdk-dev] [PATCH v2] devtools: fix check symbol change script

[Nithin Dabilpuram]

Fix check symbol change script to detect new diff file when
it is in between  "--- /dev/null" to "b/lib/...".
Current awk line expects line to start with "a/..."
which is not always true for all diffs.
As a result if in_map was '1' earlier, it will not be changed
to '0' and we get check patch errors which are not true.

Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
Cc: nhorman@tuxdriver.com
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Tested-by: Jerin Jacob <jerinj@marvell.com>
---

v2:
- Updated logic from David to fix "not map" check 

 devtools/check-symbol-change.sh | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index c5434f3..ed2178e 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -17,13 +17,11 @@ build_map_changes()
 		# map files are altered, and all section/symbol names
 		# appearing between a triggering of this rule and the
 		# next trigger of this rule are associated with this file
-		/[-+] a\/.*\.map/ {map=$2; in_map=1}
+		/[-+] [ab]\/.*\.map/ {map=$2; in_map=1; next}
 
-		# Same pattern as above, only it matches on anything that
-		# does not end in 'map', indicating we have left the map chunk.
-		# When we hit this, turn off the in_map variable, which
-		# supresses the subordonate rules below
-		/[-+] a\/.*\.[^map]/ {in_map=0}
+		# The previous rule catches all .map files, anything else
+		# indicates we left the map chunk.
+		/[-+] [ab]\// {in_map=0}
 
 		# Triggering this rule, which starts a line and ends it
 		# with a { identifies a versioned section.  The section name is
-- 
2.8.4

Re: [dpdk-dev] [PATCH v2] devtools: fix check symbol change script

[David Marchand]

On Mon, Mar 23, 2020 at 12:56 PM Nithin Dabilpuram
<ndabilpuram@marvell.com> wrote:
>
> Fix check symbol change script to detect new diff file when
> it is in between  "--- /dev/null" to "b/lib/...".
> Current awk line expects line to start with "a/..."
> which is not always true for all diffs.
> As a result if in_map was '1' earlier, it will not be changed
> to '0' and we get check patch errors which are not true.
>
> Fixes: 4bec48184e33 ("devtools: add checks for ABI symbol addition")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Nithin Dabilpuram <ndabilpuram@marvell.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Tested-by: Jerin Jacob <jerinj@marvell.com>

Thanks Nithin, applied.

-- 
David Marchand