Merge ~rafaeldtinoco/ubuntu/+source/iproute2:lp1831775-cosmic-sru-iproute2 into ubuntu/+source/iproute2:ubuntu/cosmic-devel

[Rafael David Tinoco]

Cosmic Fix for issue: https://bugs.launchpad.net/ubuntu/+source/iproute2/+bug/1831775

[Andreas Hasenack (ahasenack)]

Thanks, some comments inline, and below.

For SRUs, the merge target should always be ubuntu/<release>-devel.

Can you elaborate a bit more on the test case you added to the bug? Like, what to expect in the broken and in the good cases. Ideally it should be as simple as just copying and pasting something in the command line, but it's fine if it needs some setting up. For example, "create a connection from A to B, note the IPs used, and use the IPs in the following command."

I saw you added some test results further down in the bug, but for the SRU template, it's best if all that's needed to verify the bug is fixed is in the "opening salvo".

In "regression potential", you say " * ss interpreter (bison powered) could be broken". What does that mean? Why do you think it could be broken? These are questions the SRU team would probably ask. "ip" is used all over the place, and it's good to detail what are your thoughts on possible regressions.

review: Needs Fixing

[Rafael David Tinoco (rafaeldtinoco)]

Hello Andreas,

I pushed the commit again fixing changelog and version for cosmic which I saw it was wrong. Now, after reading your comments..

"""
Thanks, some comments inline, and below.

For SRUs, the merge target should always be ubuntu/<release>-devel.
"""

Okay

"""
Can you elaborate a bit more on the test case you added to the bug? Like, what to expect in the broken and in the good cases. Ideally it should be as simple as just copying and pasting something in the command line, but it's fine if it needs some setting up. For example, "create a connection from A to B, note the IPs used, and use the IPs in the following command."
"""

Alright, will change bug description for a more clear way of reproducing the issue

"""
I saw you added some test results further down in the bug, but for the SRU template, it's best if all that's needed to verify the bug is fixed is in the "opening salvo".
"""

I see.

"""
In "regression potential", you say " * ss interpreter (bison powered) could be broken". What does that mean? Why do you think it could be broken? These are questions the SRU team would probably ask. "ip" is used all over the place, and it's good to detail what are your thoughts on possible regressions.
"""

I usually start with biggest risk and try to reduce the risk with other topics. So, in this case:

 * ss interpreter (bison powered) could be broken

Means that the biggest risk would be to have ss interpreter broken.

 * patch is based in an upstream fix and was tested

But the patch, that changes ss interpreter, is an upstream fix and has been tested.

 * would likely not jeopardize ip command (higher issues)

It would NOT affect "ip" command and its interpreter, the most important part of iproute2.

But , now that I re-wrote in other words, I see the need to change that and will revisit.

[Rafael David Tinoco (rafaeldtinoco)]

I re-pushed the patch, fixed the versioning for 4.18.0-1ubuntu2.18.10.1, to differentiate it from Disco and Eoan merge requests I'll send soon. I have also fixed the changelog description (it had signed-off altogether and missing initial spaces, etc).

I'm trying to change the MR to -devel (instead of -proposed).

Thanks!

6da6ddb... by Rafael David Tinoco on 2019-06-06

Add: debian/patches/ss-review-ssfilter.patch

Fixed issue with ss and ssfilter where ssfilter rejects single
expresions if enclosed are in braces (LP: #1831775)

Signed-off-by: Rafael David Tinoco <email address hidden>

227676a... by Rafael David Tinoco on 2019-06-06

updated changelog for 4.18.0-1ubuntu2.18.10.1

Signed-off-by: Rafael David Tinoco <email address hidden>

Unmerged commits

227676a... by Rafael David Tinoco on 2019-06-06

updated changelog for 4.18.0-1ubuntu2.18.10.1

Signed-off-by: Rafael David Tinoco <email address hidden>

6da6ddb... by Rafael David Tinoco on 2019-06-06

Add: debian/patches/ss-review-ssfilter.patch

Fixed issue with ss and ssfilter where ssfilter rejects single
expresions if enclosed are in braces (LP: #1831775)

Signed-off-by: Rafael David Tinoco <email address hidden>

diff --git a/debian/changelog b/debian/changelog
index 9673707..961c76e 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+iproute2 (4.18.0-1ubuntu2.18.10.1) cosmic; urgency=medium
+
+  * d/p/ss-review-ssfilter.patch: fixed issue with ss and ssfilter
+    where ssfilter rejects single expresions if enclosed are in
+    braces (LP: #1831775)
+
+ -- Rafael David Tinoco <[e-mail]>  Wed, 05 Jun 2019 21:49:03 -0300
+
 iproute2 (4.18.0-1ubuntu2) cosmic; urgency=low
 
   * d/p/1006-ubuntu-iprule-fix-output.patch
diff --git a/debian/patches/series b/debian/patches/series
index 5aa32c6..cd6393b 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -7,3 +7,5 @@
 1002-ubuntu-poc-fan-driver-vxlan.patch
 1005-ubuntu-fix-testsuite-kenv.patch
 1006-ubuntu-iprule-fix-output.patch
+
+ss-review-ssfilter.patch
diff --git a/debian/patches/ss-review-ssfilter.patch b/debian/patches/ss-review-ssfilter.patch
 new file mode 100644
index 0000000..d11928d
--- /dev/null
+++ b/debian/patches/ss-review-ssfilter.patch
@@ -0,0 +1,129 @@
+Description: ss: Review ssfilter
+
+The original problem was ssfilter rejecting single expressions if
+enclosed in braces, such as:
+
+| sport = 22 or ( dport = 22 )
+
+This is fixed by allowing 'expr' to be an 'exprlist' enclosed in braces.
+The no longer required recursion in 'exprlist' being an 'exprlist'
+enclosed in braces is dropped.
+
+In addition to that, a few other things are changed:
+
+* Remove pointless 'null' prefix in 'appled' before 'exprlist'.
+* For simple equals matches, '=' operator was required for ports but not
+  allowed for hosts. Make this consistent by making '=' operator
+  optional in both cases.
+
+Reported-by: Samuel Mannehed <[e-mail]>
+Fixes: b2038cc0b2403 ("ssfilter: Eliminate shift/reduce conflicts")
+Signed-off-by: Phil Sutter <[e-mail]>
+Signed-off-by: Stephen Hemminger <[e-mail]>
+
+Origin: https://github.com/shemminger/iproute2/commit/38d209ecf2ae
+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1831775
+Last-Update: 2019-06-05
+---
+ misc/ssfilter.y | 36 +++++++++++++++++++++---------------
+ 1 file changed, 21 insertions(+), 15 deletions(-)
+
+diff --git a/misc/ssfilter.y b/misc/ssfilter.y
+index 88d4229a..0413ddda 100644
+--- a/misc/ssfilter.y
++++ b/misc/ssfilter.y
+@@ -42,24 +42,22 @@ static void yyerror(char *s)
+ %nonassoc '!'
+ 
+ %%
+-applet: null exprlist
++applet: exprlist
+         {
+-                *yy_ret = $2;
+-                $$ = $2;
++                *yy_ret = $1;
++                $$ = $1;
+         }
+         | null
+         ;
++
+ null:   /* NOTHING */ { $$ = NULL; }
+         ;
++
+ exprlist: expr
+         | '!' expr
+         {
+                 $$ = alloc_node(SSF_NOT, $2);
+         }
+-        | '(' exprlist ')'
+-        {
+-                $$ = $2;
+-        }
+         | exprlist '|' expr
+         {
+                 $$ = alloc_node(SSF_OR, $1);
+@@ -77,13 +75,21 @@ exprlist: expr
+         }
+         ;
+ 
+-expr:	DCOND HOSTCOND
++eq:	'='
++	| /* nothing */
++	;
++
++expr:	'(' exprlist ')'
++	{
++		$$ = $2;
++	}
++	| DCOND eq HOSTCOND
+         {
+-		$$ = alloc_node(SSF_DCOND, $2);
++		$$ = alloc_node(SSF_DCOND, $3);
+         }
+-        | SCOND HOSTCOND
++        | SCOND eq HOSTCOND
+         {
+-		$$ = alloc_node(SSF_SCOND, $2);
++		$$ = alloc_node(SSF_SCOND, $3);
+         }
+         | DPORT GEQ HOSTCOND
+         {
+@@ -101,7 +107,7 @@ expr:	DCOND HOSTCOND
+         {
+                 $$ = alloc_node(SSF_NOT, alloc_node(SSF_D_GE, $3));
+         }
+-        | DPORT '=' HOSTCOND
++        | DPORT eq HOSTCOND
+         {
+ 		$$ = alloc_node(SSF_DCOND, $3);
+         }
+@@ -126,7 +132,7 @@ expr:	DCOND HOSTCOND
+         {
+                 $$ = alloc_node(SSF_NOT, alloc_node(SSF_S_GE, $3));
+         }
+-        | SPORT '=' HOSTCOND
++        | SPORT eq HOSTCOND
+         {
+ 		$$ = alloc_node(SSF_SCOND, $3);
+         }
+@@ -134,7 +140,7 @@ expr:	DCOND HOSTCOND
+         {
+ 		$$ = alloc_node(SSF_NOT, alloc_node(SSF_SCOND, $3));
+         }
+-        | DEVNAME '=' DEVCOND
++        | DEVNAME eq DEVCOND
+         {
+ 		$$ = alloc_node(SSF_DEVCOND, $3);
+         }
+@@ -142,7 +148,7 @@ expr:	DCOND HOSTCOND
+         {
+ 		$$ = alloc_node(SSF_NOT, alloc_node(SSF_DEVCOND, $3));
+         }
+-        | FWMARK '=' MARKMASK
++        | FWMARK eq MARKMASK
+         {
+                 $$ = alloc_node(SSF_MARKMASK, $3);
+         }
+-- 
+2.20.1
+