[dpdk-dev] [PATCH] devtools/check-symbol-changes.sh: awk script issue

[dpdk-dev] [PATCH] devtools/check-symbol-changes.sh: awk script issue

[Liang Ma]

There is a issue inside check-symbol-changes.sh awk script.
When the script try to parse the section name from  patch,
The script put char "+" into the section name.

Signed-off-by: Liang Ma <liang.j.ma@intel.com>
---
 devtools/check-symbol-change.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
index c0d2a6d..4a0f4d8 100755
--- a/devtools/check-symbol-change.sh
+++ b/devtools/check-symbol-change.sh
@@ -30,7 +30,7 @@ build_map_changes()
 		# the rest of the line with the + and { symbols remvoed.
 		# Triggering this rule sets in_sec to 1, which actives the
 		# symbol rule below
-		/^.*{/ {
+		/^[^-+]*{/ {
 			if (in_map == 1) {
 				sec=$(NF-1); in_sec=1;
 			}
-- 
2.7.5

Re: [dpdk-dev] [PATCH] devtools/check-symbol-changes.sh: awk script issue

[Thomas Monjalon]

Hi Neil,

Are you OK with this patch?


04/10/2018 17:18, Liang Ma:
> There is a issue inside check-symbol-changes.sh awk script.
> When the script try to parse the section name from  patch,
> The script put char "+" into the section name.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> ---
>  devtools/check-symbol-change.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> index c0d2a6d..4a0f4d8 100755
> --- a/devtools/check-symbol-change.sh
> +++ b/devtools/check-symbol-change.sh
> @@ -30,7 +30,7 @@ build_map_changes()
>  		# the rest of the line with the + and { symbols remvoed.
>  		# Triggering this rule sets in_sec to 1, which actives the
>  		# symbol rule below
> -		/^.*{/ {
> +		/^[^-+]*{/ {
>  			if (in_map == 1) {
>  				sec=$(NF-1); in_sec=1;
>  			}

Re: [dpdk-dev] [PATCH] devtools/check-symbol-changes.sh: awk script issue

[Neil Horman]

On Fri, Oct 19, 2018 at 12:48:57PM +0200, Thomas Monjalon wrote:
> Hi Neil,
> 
> Are you OK with this patch?
> 
> 
> 04/10/2018 17:18, Liang Ma:
> > There is a issue inside check-symbol-changes.sh awk script.
> > When the script try to parse the section name from  patch,
> > The script put char "+" into the section name.
> > 
> > Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> > ---
> >  devtools/check-symbol-change.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > index c0d2a6d..4a0f4d8 100755
> > --- a/devtools/check-symbol-change.sh
> > +++ b/devtools/check-symbol-change.sh
> > @@ -30,7 +30,7 @@ build_map_changes()
> >  		# the rest of the line with the + and { symbols remvoed.
> >  		# Triggering this rule sets in_sec to 1, which actives the
> >  		# symbol rule below
> > -		/^.*{/ {
> > +		/^[^-+]*{/ {
> >  			if (in_map == 1) {
> >  				sec=$(NF-1); in_sec=1;
> >  			}
> 
> 
> 
> 

I don't think so.  The section name might get a + in front of it on lines like
this:

+EXPERIMENTAL {

But the above change just skips matching on it, which I don't think is what we
want.  The right fix I think would be to match on it, then strip the leading
plus with a sed operation in the body of the match rule.

Neil

Re: [dpdk-dev] [PATCH] devtools/check-symbol-changes.sh: awk script issue

[Liang, Ma]

Hi Neil, 
   there are two things here. 
   1. This issue give me negative report when I run checkpatch.
      So, I hope we can disable symbol check before we fix it.

   2. How to fix that 
      I still don't understand why we need match section name start with '+'.
      The section name should start with 'A-Z' only for my opinion.

Regards
Liang

On 19 Oct 07:38, Neil Horman wrote:
> On Fri, Oct 19, 2018 at 12:48:57PM +0200, Thomas Monjalon wrote:
> > Hi Neil,
> > 
> > Are you OK with this patch?
> > 
> > 
> > 04/10/2018 17:18, Liang Ma:
> > > There is a issue inside check-symbol-changes.sh awk script.
> > > When the script try to parse the section name from  patch,
> > > The script put char "+" into the section name.
> > > 
> > > Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> > > ---
> > >  devtools/check-symbol-change.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > > index c0d2a6d..4a0f4d8 100755
> > > --- a/devtools/check-symbol-change.sh
> > > +++ b/devtools/check-symbol-change.sh
> > > @@ -30,7 +30,7 @@ build_map_changes()
> > >  		# the rest of the line with the + and { symbols remvoed.
> > >  		# Triggering this rule sets in_sec to 1, which actives the
> > >  		# symbol rule below
> > > -		/^.*{/ {
> > > +		/^[^-+]*{/ {
> > >  			if (in_map == 1) {
> > >  				sec=$(NF-1); in_sec=1;
> > >  			}
> > 
> > 
> > 
> > 
> 
> I don't think so.  The section name might get a + in front of it on lines like
> this:
> 
> +EXPERIMENTAL {
> 
> But the above change just skips matching on it, which I don't think is what we
> want.  The right fix I think would be to match on it, then strip the leading
> plus with a sed operation in the body of the match rule.
> 
> Neil
> 

Re: [dpdk-dev] [PATCH] devtools/check-symbol-changes.sh: awk script issue

[Neil Horman]

On Fri, Oct 19, 2018 at 02:09:51PM +0100, Liang, Ma wrote:
> Hi Neil, 
>    there are two things here. 
>    1. This issue give me negative report when I run checkpatch.
>       So, I hope we can disable symbol check before we fix it.
> 
You're welcome to disable the check locally I suppose, but I'll not
agree to a patch that does that.  I understand you have a false negative report,
but the right thing to do is fix it.  I appreciate you wanting to do that, but I
don't think what you have below is the right answer. 

>    2. How to fix that 
>       I still don't understand why we need match section name start with '+'.
>       The section name should start with 'A-Z' only for my opinion.
> 
We don't, thats not what I said.  Your patch changes the match rule to
enforce not having a + or - in the lead of the line defining the section.  But,
as the patch you triggered on shows, that might be the line which defines what
the section is, so instead of ignoring those lines (which this patch does), what
I suggests was keeping the match line exactly as is, and add a line in the
actions to strip the + or - out, something like this:

/^.*{/ {
	...
	sec=$(NF-1);sec=`echo $sec | sed -e"s/[+-]//g"; in_sec=1
}

that way you get section names when they're changing, and still can remove the
leading cruft

Regards
Neil
> Liang
> 
> On 19 Oct 07:38, Neil Horman wrote:
> > On Fri, Oct 19, 2018 at 12:48:57PM +0200, Thomas Monjalon wrote:
> > > Hi Neil,
> > > 
> > > Are you OK with this patch?
> > > 
> > > 
> > > 04/10/2018 17:18, Liang Ma:
> > > > There is a issue inside check-symbol-changes.sh awk script.
> > > > When the script try to parse the section name from  patch,
> > > > The script put char "+" into the section name.
> > > > 
> > > > Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> > > > ---
> > > >  devtools/check-symbol-change.sh | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/devtools/check-symbol-change.sh b/devtools/check-symbol-change.sh
> > > > index c0d2a6d..4a0f4d8 100755
> > > > --- a/devtools/check-symbol-change.sh
> > > > +++ b/devtools/check-symbol-change.sh
> > > > @@ -30,7 +30,7 @@ build_map_changes()
> > > >  		# the rest of the line with the + and { symbols remvoed.
> > > >  		# Triggering this rule sets in_sec to 1, which actives the
> > > >  		# symbol rule below
> > > > -		/^.*{/ {
> > > > +		/^[^-+]*{/ {
> > > >  			if (in_map == 1) {
> > > >  				sec=$(NF-1); in_sec=1;
> > > >  			}
> > > 
> > > 
> > > 
> > > 
> > 
> > I don't think so.  The section name might get a + in front of it on lines like
> > this:
> > 
> > +EXPERIMENTAL {
> > 
> > But the above change just skips matching on it, which I don't think is what we
> > want.  The right fix I think would be to match on it, then strip the leading
> > plus with a sed operation in the body of the match rule.
> > 
> > Neil
> > 
>