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

Proposed by Rafael David Tinoco
Status: Merged
Approved by: Andreas Hasenack
Approved revision: 96b1b4a6b461effe47a19d5d62d9f0e9825e9fcb
Merged at revision: 96b1b4a6b461effe47a19d5d62d9f0e9825e9fcb
Proposed branch: ~rafaeldtinoco/ubuntu/+source/iproute2:lp1831775-eoan-sru-iproute2
Merge into: ubuntu/+source/iproute2:ubuntu/eoan-devel
Diff against target: 160 lines (+138/-0)
3 files modified
debian/changelog (+8/-0)
debian/patches/series (+2/-0)
debian/patches/ss-review-ssfilter.patch (+128/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack Approve
Canonical Server Core Reviewers Pending
Canonical Server Pending
Review via email: mp+368420@code.launchpad.net

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

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

Some comments inline.

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

Thanks for reviewing it. Fixing and pushing it again.

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

Thanks for reviewing v1 @ahasenack, sent out a v2 and re-submitted patch.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

You don't need to resubmit the whole mp. Just make the changes and push.
It's also usually fine to force push, unless the changes are big, then you
should probably commit on top and just at the last step squash it together
before the upload and merge.

On Wed, Jun 5, 2019, 21:32 Rafael David Tinoco <email address hidden>
wrote:

> Thanks for reviewing v1 @ahasenack, sent out a v2 and re-submitted patch.
> --
>
> https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/iproute2/+git/iproute2/+merge/368420
> You are requested to review the proposed merge of
> ~rafaeldtinoco/ubuntu/+source/iproute2:lp1831775-eoan-sru-iproute2 into
> ubuntu/+source/iproute2:ubuntu/eoan-devel.
>
> Launchpad-Message-Rationale: Reviewer
> Launchpad-Message-For: ahasenack
> Launchpad-Notification-Type: code-review
> Launchpad-Branch:
> ~rafaeldtinoco/ubuntu/+source/iproute2/+git/iproute2:lp1831775-eoan-sru-iproute2
>

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

Ah, good to know. I thought because I was using -f I'd had to resubmit.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Tiny comment inside about the dep3 header.

I noticed that the package has dep8 tests, could you run them and see if they pass?

I'd do something like:

autopkgtest -o dep8-results -U -s --setup-commands="sudo apt update; sudo apt install -y python-minimal" ./iproute2/ -- qemu <path-to-eoan-autopkgtest-image>

-U: update packages
-s: give you a shell if it fails
--setup-commands: something that I seem to have to do recently with eoan images, otherwise autopkgtest fails because it doesn't find /usr/bin/python (might be my image only that has this problem)

Giving it the path to the iproute2 checkout will tell it to build the source, and then test. Since iproute2 builds quickly, that's fine. Otherwise you might want to upload it to a PPA and use the build from there once it's finished.

Also, in the team we like to try out the new package, so it helps if you can prep a ppa for this bug and upload iproute2 there, with a ~ppaN suffix.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

spoiler alert: the tests passed
But it's good practice to run them when they exist. It avoids surprises later on during migration, when it could be blocking other packages and suddenly becomes an urgent fix.

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

Alright, will run the DEP8 tests and provide the PPA.

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

Added a PPA for review:

https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1831775

But uploaded same version (instead of using ~lpXXXX~1). Will re-do it with same name.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Thanks for the ppa and SRU testing instructions updates. Went through them and they are good.

Just a tiny typo in the d/changelog and corresponding commit, and we are good to go.

Verified:
- d/changelog version and release
- patch dep3
- patch is same as upstream's
- SRU test case

+1 after typo fix in d/changelog

Revision history for this message
Rafael David Tinoco (rafaeldtinoco) wrote :

@ahasenack,

Tks a lot for the review, fixed the typo. I'm also cherry-picking those 2 fixes to the other merges now: Disco and Cosmic

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Looks good, +1

review: Approve
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Tag pushed and package uploaded:
$ git push pkg upload/4.18.0-1ubuntu3
Enumerating objects: 16, done.
Counting objects: 100% (16/16), done.
Delta compression using up to 2 threads
Compressing objects: 100% (11/11), done.
Writing objects: 100% (11/11), 2.51 KiB | 321.00 KiB/s, done.
Total 11 (delta 7), reused 0 (delta 0)
To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/iproute2
 * [new tag] upload/4.18.0-1ubuntu3 -> upload/4.18.0-1ubuntu3

$ dput ubuntu ../iproute2_4.18.0-1ubuntu3_source.changes
Checking signature on .changes
gpg: ../iproute2_4.18.0-1ubuntu3_source.changes: Valid signature from AC983EB5BF6BCBA9
Checking signature on .dsc
gpg: ../iproute2_4.18.0-1ubuntu3.dsc: Valid signature from AC983EB5BF6BCBA9
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading iproute2_4.18.0-1ubuntu3.dsc: done.
  Uploading iproute2_4.18.0-1ubuntu3.debian.tar.xz: done.
  Uploading iproute2_4.18.0-1ubuntu3_source.buildinfo: done.
  Uploading iproute2_4.18.0-1ubuntu3_source.changes: done.
Successfully uploaded packages.

Please follow its migration in https://people.canonical.com/~ubuntu-archive/proposed-migration/update_excuses.html

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..3e4135f 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,11 @@
6+iproute2 (4.18.0-1ubuntu3) eoan; urgency=medium
7+
8+ * d/p/ss-review-ssfilter.patch: fixed issue with ss and ssfilter
9+ where ssfilter rejects single expressions if enclosed in
10+ braces (LP: #1831775)
11+
12+ -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Wed, 05 Jun 2019 21:26:00 -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..749b114
30--- /dev/null
31+++ b/debian/patches/ss-review-ssfilter.patch
32@@ -0,0 +1,128 @@
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: upstream, https://github.com/shemminger/iproute2/commit/38d209ecf2ae
57+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1831775
58+---
59+ misc/ssfilter.y | 36 +++++++++++++++++++++---------------
60+ 1 file changed, 21 insertions(+), 15 deletions(-)
61+
62+diff --git a/misc/ssfilter.y b/misc/ssfilter.y
63+index 88d4229a..0413ddda 100644
64+--- a/misc/ssfilter.y
65++++ b/misc/ssfilter.y
66+@@ -42,24 +42,22 @@ static void yyerror(char *s)
67+ %nonassoc '!'
68+
69+ %%
70+-applet: null exprlist
71++applet: exprlist
72+ {
73+- *yy_ret = $2;
74+- $$ = $2;
75++ *yy_ret = $1;
76++ $$ = $1;
77+ }
78+ | null
79+ ;
80++
81+ null: /* NOTHING */ { $$ = NULL; }
82+ ;
83++
84+ exprlist: expr
85+ | '!' expr
86+ {
87+ $$ = alloc_node(SSF_NOT, $2);
88+ }
89+- | '(' exprlist ')'
90+- {
91+- $$ = $2;
92+- }
93+ | exprlist '|' expr
94+ {
95+ $$ = alloc_node(SSF_OR, $1);
96+@@ -77,13 +75,21 @@ exprlist: expr
97+ }
98+ ;
99+
100+-expr: DCOND HOSTCOND
101++eq: '='
102++ | /* nothing */
103++ ;
104++
105++expr: '(' exprlist ')'
106++ {
107++ $$ = $2;
108++ }
109++ | DCOND eq HOSTCOND
110+ {
111+- $$ = alloc_node(SSF_DCOND, $2);
112++ $$ = alloc_node(SSF_DCOND, $3);
113+ }
114+- | SCOND HOSTCOND
115++ | SCOND eq HOSTCOND
116+ {
117+- $$ = alloc_node(SSF_SCOND, $2);
118++ $$ = alloc_node(SSF_SCOND, $3);
119+ }
120+ | DPORT GEQ HOSTCOND
121+ {
122+@@ -101,7 +107,7 @@ expr: DCOND HOSTCOND
123+ {
124+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_D_GE, $3));
125+ }
126+- | DPORT '=' HOSTCOND
127++ | DPORT eq HOSTCOND
128+ {
129+ $$ = alloc_node(SSF_DCOND, $3);
130+ }
131+@@ -126,7 +132,7 @@ expr: DCOND HOSTCOND
132+ {
133+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_S_GE, $3));
134+ }
135+- | SPORT '=' HOSTCOND
136++ | SPORT eq HOSTCOND
137+ {
138+ $$ = alloc_node(SSF_SCOND, $3);
139+ }
140+@@ -134,7 +140,7 @@ expr: DCOND HOSTCOND
141+ {
142+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_SCOND, $3));
143+ }
144+- | DEVNAME '=' DEVCOND
145++ | DEVNAME eq DEVCOND
146+ {
147+ $$ = alloc_node(SSF_DEVCOND, $3);
148+ }
149+@@ -142,7 +148,7 @@ expr: DCOND HOSTCOND
150+ {
151+ $$ = alloc_node(SSF_NOT, alloc_node(SSF_DEVCOND, $3));
152+ }
153+- | FWMARK '=' MARKMASK
154++ | FWMARK eq MARKMASK
155+ {
156+ $$ = alloc_node(SSF_MARKMASK, $3);
157+ }
158+--
159+2.20.1
160+

Subscribers

People subscribed via source and target branches