patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] devtools: fix check symbol change script
@ 2020-03-19 14:44 Nithin Dabilpuram
  2020-03-19 14:56 ` David Marchand
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Nithin Dabilpuram @ 2020-03-19 14:44 UTC (permalink / raw)
  To: thomas, david.marchand, Neil Horman
  Cc: dev, jerinj, Nithin Dabilpuram, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] devtools: fix check symbol change script
  2020-03-19 14:44 [dpdk-stable] [PATCH] devtools: fix check symbol change script Nithin Dabilpuram
@ 2020-03-19 14:56 ` David Marchand
  2020-03-19 15:40   ` Neil Horman
  2020-03-22 14:37 ` [dpdk-stable] [dpdk-dev] " Jerin Jacob
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2020-03-19 14:56 UTC (permalink / raw)
  To: Neil Horman
  Cc: Thomas Monjalon, dev, Jerin Jacob Kollanukkaran,
	Nithin Dabilpuram, dpdk stable, bingz

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
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] devtools: fix check symbol change script
  2020-03-19 14:56 ` David Marchand
@ 2020-03-19 15:40   ` Neil Horman
  2020-03-19 15:45     ` [dpdk-stable] [EXT] " Nithin Dabilpuram
  2020-03-19 15:49     ` [dpdk-stable] " Bing Zhao
  0 siblings, 2 replies; 12+ messages in thread
From: Neil Horman @ 2020-03-19 15:40 UTC (permalink / raw)
  To: David Marchand
  Cc: Thomas Monjalon, dev, Jerin Jacob Kollanukkaran,
	Nithin Dabilpuram, dpdk stable, bingz

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
> >
> 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [EXT] Re: [PATCH] devtools: fix check symbol change script
  2020-03-19 15:40   ` Neil Horman
@ 2020-03-19 15:45     ` Nithin Dabilpuram
  2020-03-19 18:59       ` Neil Horman
  2020-03-19 15:49     ` [dpdk-stable] " Bing Zhao
  1 sibling, 1 reply; 12+ messages in thread
From: Nithin Dabilpuram @ 2020-03-19 15:45 UTC (permalink / raw)
  To: Neil Horman
  Cc: David Marchand, Thomas Monjalon, dev, Jerin Jacob Kollanukkaran,
	dpdk stable, bingz

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
> > >
> > 
> > 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] devtools: fix check symbol change script
  2020-03-19 15:40   ` Neil Horman
  2020-03-19 15:45     ` [dpdk-stable] [EXT] " Nithin Dabilpuram
@ 2020-03-19 15:49     ` Bing Zhao
  1 sibling, 0 replies; 12+ messages in thread
From: Bing Zhao @ 2020-03-19 15:49 UTC (permalink / raw)
  To: Neil Horman, David Marchand
  Cc: Thomas Monjalon, dev, Jerin Jacob Kollanukkaran,
	Nithin Dabilpuram, dpdk stable

> -----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
> > >
> >
> >

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [EXT] Re: [PATCH] devtools: fix check symbol change script
  2020-03-19 15:45     ` [dpdk-stable] [EXT] " Nithin Dabilpuram
@ 2020-03-19 18:59       ` Neil Horman
  0 siblings, 0 replies; 12+ messages in thread
From: Neil Horman @ 2020-03-19 18:59 UTC (permalink / raw)
  To: Nithin Dabilpuram
  Cc: David Marchand, Thomas Monjalon, dev, Jerin Jacob Kollanukkaran,
	dpdk stable, bingz

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
> > > >
> > > 
> > > 
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] devtools: fix check symbol change script
  2020-03-19 14:44 [dpdk-stable] [PATCH] devtools: fix check symbol change script Nithin Dabilpuram
  2020-03-19 14:56 ` David Marchand
@ 2020-03-22 14:37 ` Jerin Jacob
  2020-03-23  8:13 ` [dpdk-stable] " David Marchand
  2020-03-23 11:56 ` [dpdk-stable] [PATCH v2] " Nithin Dabilpuram
  3 siblings, 0 replies; 12+ messages in thread
From: Jerin Jacob @ 2020-03-22 14:37 UTC (permalink / raw)
  To: Nithin Dabilpuram
  Cc: Thomas Monjalon, David Marchand, Neil Horman, dpdk-dev,
	Jerin Jacob, dpdk stable

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH] devtools: fix check symbol change script
  2020-03-19 14:44 [dpdk-stable] [PATCH] devtools: fix check symbol change script Nithin Dabilpuram
  2020-03-19 14:56 ` David Marchand
  2020-03-22 14:37 ` [dpdk-stable] [dpdk-dev] " Jerin Jacob
@ 2020-03-23  8:13 ` David Marchand
  2020-03-23  9:28   ` [dpdk-stable] [EXT] " Nithin Dabilpuram
  2020-03-23 11:56 ` [dpdk-stable] [PATCH v2] " Nithin Dabilpuram
  3 siblings, 1 reply; 12+ messages in thread
From: David Marchand @ 2020-03-23  8:13 UTC (permalink / raw)
  To: Nithin Dabilpuram
  Cc: Thomas Monjalon, Neil Horman, dev, Jerin Jacob Kollanukkaran,
	dpdk stable, bingz

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [EXT] Re: [PATCH] devtools: fix check symbol change script
  2020-03-23  8:13 ` [dpdk-stable] " David Marchand
@ 2020-03-23  9:28   ` Nithin Dabilpuram
  2020-03-23  9:30     ` David Marchand
  0 siblings, 1 reply; 12+ messages in thread
From: Nithin Dabilpuram @ 2020-03-23  9:28 UTC (permalink / raw)
  To: David Marchand
  Cc: Thomas Monjalon, Neil Horman, dev, Jerin Jacob Kollanukkaran,
	dpdk stable, bingz

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [EXT] Re: [PATCH] devtools: fix check symbol change script
  2020-03-23  9:28   ` [dpdk-stable] [EXT] " Nithin Dabilpuram
@ 2020-03-23  9:30     ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2020-03-23  9:30 UTC (permalink / raw)
  To: Nithin Dabilpuram
  Cc: Thomas Monjalon, Neil Horman, dev, Jerin Jacob Kollanukkaran,
	dpdk stable, bingz

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [dpdk-stable] [PATCH v2] devtools: fix check symbol change script
  2020-03-19 14:44 [dpdk-stable] [PATCH] devtools: fix check symbol change script Nithin Dabilpuram
                   ` (2 preceding siblings ...)
  2020-03-23  8:13 ` [dpdk-stable] " David Marchand
@ 2020-03-23 11:56 ` Nithin Dabilpuram
  2020-03-23 13:19   ` David Marchand
  3 siblings, 1 reply; 12+ messages in thread
From: Nithin Dabilpuram @ 2020-03-23 11:56 UTC (permalink / raw)
  To: thomas, david.marchand, Neil Horman
  Cc: dev, jerinj, bingz, Nithin Dabilpuram, stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [dpdk-stable] [PATCH v2] devtools: fix check symbol change script
  2020-03-23 11:56 ` [dpdk-stable] [PATCH v2] " Nithin Dabilpuram
@ 2020-03-23 13:19   ` David Marchand
  0 siblings, 0 replies; 12+ messages in thread
From: David Marchand @ 2020-03-23 13:19 UTC (permalink / raw)
  To: Nithin Dabilpuram
  Cc: Thomas Monjalon, Neil Horman, dev, Jerin Jacob Kollanukkaran,
	bingz, dpdk stable

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-03-23 13:19 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-19 14:44 [dpdk-stable] [PATCH] devtools: fix check symbol change script Nithin Dabilpuram
2020-03-19 14:56 ` David Marchand
2020-03-19 15:40   ` Neil Horman
2020-03-19 15:45     ` [dpdk-stable] [EXT] " Nithin Dabilpuram
2020-03-19 18:59       ` Neil Horman
2020-03-19 15:49     ` [dpdk-stable] " Bing Zhao
2020-03-22 14:37 ` [dpdk-stable] [dpdk-dev] " Jerin Jacob
2020-03-23  8:13 ` [dpdk-stable] " David Marchand
2020-03-23  9:28   ` [dpdk-stable] [EXT] " Nithin Dabilpuram
2020-03-23  9:30     ` David Marchand
2020-03-23 11:56 ` [dpdk-stable] [PATCH v2] " Nithin Dabilpuram
2020-03-23 13:19   ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).