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

Proposed by Rafael David Tinoco
Status: Superseded
Proposed branch: ~rafaeldtinoco/ubuntu/+source/iproute2:lp1831775-cosmic-sru-iproute2
Merge into: ubuntu/+source/iproute2:ubuntu/cosmic-devel
Diff against target: 161 lines (+139/-0)
3 files modified
debian/changelog (+8/-0)
debian/patches/series (+2/-0)
debian/patches/ss-review-ssfilter.patch (+129/-0)
Reviewer Review Type Date Requested Status
Canonical Server Core Reviewers Pending
Canonical Server Pending
Andreas Hasenack Pending
Review via email: mp+368408@code.launchpad.net

This proposal supersedes a proposal from 2019-06-05.

Description of the change

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote : Posted in a previous version of this proposal

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

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

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

updated changelog for 4.18.0-1ubuntu2.18.10.1

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

6da6ddb... by Rafael David Tinoco

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>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 9673707..961c76e 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,11 @@
6+iproute2 (4.18.0-1ubuntu2.18.10.1) cosmic; urgency=medium
7+
8+ * d/p/ss-review-ssfilter.patch: fixed issue with ss and ssfilter
9+ where ssfilter rejects single expresions if enclosed are in
10+ braces (LP: #1831775)
11+
12+ -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Wed, 05 Jun 2019 21:49:03 -0300
13+
14 iproute2 (4.18.0-1ubuntu2) cosmic; urgency=low
15
16 * d/p/1006-ubuntu-iprule-fix-output.patch
17diff --git a/debian/patches/series b/debian/patches/series
18index 5aa32c6..cd6393b 100644
19--- a/debian/patches/series
20+++ b/debian/patches/series
21@@ -7,3 +7,5 @@
22 1002-ubuntu-poc-fan-driver-vxlan.patch
23 1005-ubuntu-fix-testsuite-kenv.patch
24 1006-ubuntu-iprule-fix-output.patch
25+
26+ss-review-ssfilter.patch
27diff --git a/debian/patches/ss-review-ssfilter.patch b/debian/patches/ss-review-ssfilter.patch
28new file mode 100644
29index 0000000..d11928d
30--- /dev/null
31+++ b/debian/patches/ss-review-ssfilter.patch
32@@ -0,0 +1,129 @@
33+Description: ss: Review ssfilter
34+
35+The original problem was ssfilter rejecting single expressions if
36+enclosed in braces, such as:
37+
38+| sport = 22 or ( dport = 22 )
39+
40+This is fixed by allowing 'expr' to be an 'exprlist' enclosed in braces.
41+The no longer required recursion in 'exprlist' being an 'exprlist'
42+enclosed in braces is dropped.
43+
44+In addition to that, a few other things are changed:
45+
46+* Remove pointless 'null' prefix in 'appled' before 'exprlist'.
47+* For simple equals matches, '=' operator was required for ports but not
48+ allowed for hosts. Make this consistent by making '=' operator
49+ optional in both cases.
50+
51+Reported-by: Samuel Mannehed <samuel@cendio.se>
52+Fixes: b2038cc0b2403 ("ssfilter: Eliminate shift/reduce conflicts")
53+Signed-off-by: Phil Sutter <phil@nwl.cc>
54+Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
55+
56+Origin: https://github.com/shemminger/iproute2/commit/38d209ecf2ae
57+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1831775
58+Last-Update: 2019-06-05
59+---
60+ misc/ssfilter.y | 36 +++++++++++++++++++++---------------
61+ 1 file changed, 21 insertions(+), 15 deletions(-)
62+
63+diff --git a/misc/ssfilter.y b/misc/ssfilter.y
64+index 88d4229a..0413ddda 100644
65+--- a/misc/ssfilter.y
66++++ b/misc/ssfilter.y
67+@@ -42,24 +42,22 @@ static void yyerror(char *s)
68+ %nonassoc '!'
69+
70+ %%
71+-applet: null exprlist
72++applet: exprlist
73+ {
74+- *yy_ret = $2;
75+- $$ = $2;
76++ *yy_ret = $1;
77++ $$ = $1;
78+ }
79+ | null
80+ ;
81++
82+ null: /* NOTHING */ { $$ = NULL; }
83+ ;
84++
85+ exprlist: expr
86+ | '!' expr
87+ {
88+ $$ = alloc_node(SSF_NOT, $2);
89+ }
90+- | '(' exprlist ')'
91+- {
92+- $$ = $2;
93+- }
94+ | exprlist '|' expr
95+ {
96+ $$ = alloc_node(SSF_OR, $1);
97+@@ -77,13 +75,21 @@ exprlist: expr
98+ }
99+ ;
100+
101+-expr: DCOND HOSTCOND
102++eq: '='
103++ | /* nothing */
104++ ;
105++
106++expr: '(' exprlist ')'
107++ {
108++ $$ = $2;
109++ }
110++ | DCOND eq HOSTCOND
111+ {
112+- $$ = alloc_node(SSF_DCOND, $2);
113++ $$ = alloc_node(SSF_DCOND, $3);
114+ }
115+- | SCOND HOSTCOND
116++ | SCOND eq HOSTCOND
117+ {
118+- $$ = alloc_node(SSF_SCOND, $2);
119++ $$ = alloc_node(SSF_SCOND, $3);
120+ }
121+ | DPORT GEQ HOSTCOND
122+ {
123+@@ -101,7 +107,7 @@ expr: DCOND HOSTCOND
124+ {
125+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_D_GE, $3));
126+ }
127+- | DPORT '=' HOSTCOND
128++ | DPORT eq HOSTCOND
129+ {
130+ $$ = alloc_node(SSF_DCOND, $3);
131+ }
132+@@ -126,7 +132,7 @@ expr: DCOND HOSTCOND
133+ {
134+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_S_GE, $3));
135+ }
136+- | SPORT '=' HOSTCOND
137++ | SPORT eq HOSTCOND
138+ {
139+ $$ = alloc_node(SSF_SCOND, $3);
140+ }
141+@@ -134,7 +140,7 @@ expr: DCOND HOSTCOND
142+ {
143+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_SCOND, $3));
144+ }
145+- | DEVNAME '=' DEVCOND
146++ | DEVNAME eq DEVCOND
147+ {
148+ $$ = alloc_node(SSF_DEVCOND, $3);
149+ }
150+@@ -142,7 +148,7 @@ expr: DCOND HOSTCOND
151+ {
152+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_DEVCOND, $3));
153+ }
154+- | FWMARK '=' MARKMASK
155++ | FWMARK eq MARKMASK
156+ {
157+ $$ = alloc_node(SSF_MARKMASK, $3);
158+ }
159+--
160+2.20.1
161+

Subscribers

People subscribed via source and target branches