devtools/check-symbol-changes.sh: awk script issue

Message ID 1538666296-25742-1-git-send-email-liang.j.ma@intel.com
State Rejected, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • devtools/check-symbol-changes.sh: awk script issue
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Liang, Ma Oct. 4, 2018, 3:18 p.m.
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(-)

Comments

Thomas Monjalon Oct. 19, 2018, 10:48 a.m. | #1
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;
>  			}
Neil Horman Oct. 19, 2018, 11:38 a.m. | #2
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
Liang, Ma Oct. 19, 2018, 1:09 p.m. | #3
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
>
Neil Horman Oct. 19, 2018, 8:32 p.m. | #4
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
> > 
>

Patch

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