Merge ~paelzer/ubuntu/+source/psmisc:lp1806060-fix-killall-option-parsing-bionic into ubuntu/+source/psmisc:ubuntu/bionic-devel

Proposed by Christian Ehrhardt  on 2018-12-11
Status: Merged
Approved by: Christian Ehrhardt  on 2018-12-14
Approved revision: fbe4683bb536ad101340637f62b3bb7495ecda53
Merged at revision: fbe4683bb536ad101340637f62b3bb7495ecda53
Proposed branch: ~paelzer/ubuntu/+source/psmisc:lp1806060-fix-killall-option-parsing-bionic
Merge into: ubuntu/+source/psmisc:ubuntu/bionic-devel
Diff against target: 217 lines (+185/-0)
5 files modified
debian/changelog (+9/-0)
debian/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch (+49/-0)
debian/patches/lp1806060-0002-killall-change-to-getopt_long.patch (+37/-0)
debian/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch (+87/-0)
debian/patches/series (+3/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack 2018-12-11 Approve on 2018-12-13
Canonical Server Team 2018-12-11 Pending
Ubuntu Server Dev import team 2018-12-11 Pending
Review via email: mp+360687@code.launchpad.net
To post a comment you must log in.
Christian Ehrhardt  (paelzer) wrote :

PPA verified with the testcase.
Bileto tests run on ticket: https://bileto.ubuntu.com/#/ticket/3561

This is open for review now ...

Christian Ehrhardt  (paelzer) wrote :

tests look shaky, but not due to the upload

Maria is already masked, but needs a bump to
vorlon:16:# requires its own version of mariadb-test, that binary package now owned by mariadb-10.2.
vorlon:17:force-badtest mariadb-10.1/1:10.1.29-6ubuntu2

pcs is already masked.

Remaining issues on booth, s3ql and gvfs
At least gvfs is a known trouble maker in that context (s390x), but it also fails on others now.
Retriggered the tests, none of them looks like it would be related to the proposed upload

Christian Ehrhardt  (paelzer) wrote :

Actually on Bionic the badtests are defined wider, so what is left is:
- gvfs @ i386 ppc64el
- booth @ s390x
Retrying theses for now ...

There but on older versions:
- s3ql
- mariadb
Those likely will need MPs for hints-ubuntu ...

already covered are:
- ocfs2-tools
- pcs

Christian Ehrhardt  (paelzer) wrote :

Ok all tests that I retried worked now.
Bumping the tests os s3ql and mariadb is a necessary evil that I'll open MPs once I uploaded and see the issues in -excuses
So it is ok for upload given the review is ack'ed

Andreas Hasenack (ahasenack) wrote :

Ok:

- compared cosmic fix with bionic fix, same
- ran the test (nice script), ok
- even though sometimes the test script doesn't find a process to kill, by then it parsed the command line already, and if an unknown option were to be used it would have shown the help output
- killall is definitely used by many scripts out there, let's keep an eye on possible regressions

+1

review: Approve
Andreas Hasenack (ahasenack) wrote :

Forgot to say, also verified that upstream didn't have more follow-up commits with One More Fix.

Christian Ehrhardt  (paelzer) wrote :

Thanks for the re-check Andreas.
Pushed Tag and uploaded for SRU

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 2c10bd8..2ebddaa 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+psmisc (23.1-1ubuntu0.1) bionic; urgency=medium
7+
8+ * fix killall option parsing (LP: #1806060)
9+ - d/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch
10+ - d/patches/lp1806060-0002-killall-change-to-getopt_long.patch
11+ - d/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch
12+
13+ -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Tue, 11 Dec 2018 16:46:04 +0100
14+
15 psmisc (23.1-1) unstable; urgency=medium
16
17 * New upstream source
18diff --git a/debian/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch b/debian/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch
19new file mode 100644
20index 0000000..6bb7b59
21--- /dev/null
22+++ b/debian/patches/lp1806060-0001-killall-Fix-INT-option-parsing.patch
23@@ -0,0 +1,49 @@
24+From cf80d701397d2ff705ab1c1250e3906f32034539 Mon Sep 17 00:00:00 2001
25+From: Craig Small <csmall@enc.com.au>
26+Date: Sun, 20 May 2018 20:42:12 +1000
27+Subject: [PATCH] killall: Fix -INT option parsing
28+
29+Sometimes killall gets confused between a signal name and a set of
30+options. This small patch should un-confuse it.
31+
32+Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
33+Original-Author: Craig Small <csmall@enc.com.au>
34+Origin: backport, https://gitlab.com/psmisc/psmisc/commit/cf80d701397d2ff705ab1c1250e3906f32034539
35+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1806060
36+Last-Update: 2018-12-11
37+
38+---
39+ ChangeLog | 1 +
40+ src/killall.c | 7 ++++++-
41+ 2 files changed, 7 insertions(+), 1 deletion(-)
42+
43+--- a/src/killall.c
44++++ b/src/killall.c
45+@@ -803,6 +803,7 @@ main (int argc, char **argv)
46+ int sig_num;
47+ int optc;
48+ int myoptind;
49++ int skip_error=0;
50+ struct passwd *pwent = NULL;
51+ char yt[16];
52+ char ot[16];
53+@@ -912,6 +913,7 @@ main (int argc, char **argv)
54+ ignore_case = 1;
55+ } else {
56+ sig_num = get_signal (argv[optind]+1, "killall");
57++ skip_error=1;
58+ }
59+ break;
60+ case 'V':
61+@@ -947,7 +949,10 @@ main (int argc, char **argv)
62+ if (argv[optind-1][1] >= '0' && argv[optind-1][1] <= '9') {
63+ sig_num = atoi(argv[optind-1]+1);
64+ } else {
65+- usage(NULL);
66++ if (skip_error)
67++ skip_error=0;
68++ else
69++ usage(NULL);
70+ }
71+ }
72+ break;
73diff --git a/debian/patches/lp1806060-0002-killall-change-to-getopt_long.patch b/debian/patches/lp1806060-0002-killall-change-to-getopt_long.patch
74new file mode 100644
75index 0000000..6eb56c3
76--- /dev/null
77+++ b/debian/patches/lp1806060-0002-killall-change-to-getopt_long.patch
78@@ -0,0 +1,37 @@
79+From b769da1626044f97a689c1665270595f36f878e7 Mon Sep 17 00:00:00 2001
80+From: Craig Small <csmall@enc.com.au>
81+Date: Sun, 20 May 2018 20:46:55 +1000
82+Subject: [PATCH] killall: change to getopt_long
83+
84+with getopt_long_only -ve fails because its not sure what the -v
85+is for --version or --verbose. Changing to the non-_only version means
86+it doesn't try to expand the options to long options.
87+
88+References:
89+ psmisc/psmisc#12
90+
91+Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
92+Original-Author: Craig Small <csmall@enc.com.au>
93+Origin: backport, https://gitlab.com/psmisc/psmisc/commit/b769da1626044f97a689c1665270595f36f878e7
94+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1806060
95+Last-Update: 2018-12-11
96+
97+---
98+ ChangeLog | 1 +
99+ src/killall.c | 4 ++--
100+ 2 files changed, 3 insertions(+), 2 deletions(-)
101+
102+--- a/src/killall.c
103++++ b/src/killall.c
104+@@ -856,9 +856,9 @@ main (int argc, char **argv)
105+
106+ opterr = 0;
107+ #ifdef WITH_SELINUX
108+- while ( (optc = getopt_long_only(argc,argv,"egy:o:ilqrs:u:vwZ:VIn:",options,NULL)) != -1) {
109++ while ( (optc = getopt_long(argc,argv,"egy:o:ilqrs:u:vwZ:VIn:",options,NULL)) != -1) {
110+ #else
111+- while ( (optc = getopt_long_only(argc,argv,"egy:o:ilqrs:u:vwVIn:",options,NULL)) != -1) {
112++ while ( (optc = getopt_long(argc,argv,"egy:o:ilqrs:u:vwVIn:",options,NULL)) != -1) {
113+ #endif
114+ switch (optc) {
115+ case 'e':
116diff --git a/debian/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch b/debian/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch
117new file mode 100644
118index 0000000..026f280
119--- /dev/null
120+++ b/debian/patches/lp1806060-0003-killall-Another-go-at-option-parsing.patch
121@@ -0,0 +1,87 @@
122+From 258ee9166e585f87005d3a9686938a4fa26669f9 Mon Sep 17 00:00:00 2001
123+From: Craig Small <csmall@enc.com.au>
124+Date: Tue, 18 Sep 2018 21:17:00 +1000
125+Subject: [PATCH] killall: Another go at option parsing
126+
127+This now seems to be working. There are some evil hacks, especially for
128+the -ve option combination but it seems that we have a winner.
129+
130+Added a bunch more option parsing tests which picked up -ILL passed but
131+-VTALRM did not. Not sure why, but length seems my guess
132+
133+References:
134+ psmisc/psmisc#13
135+ psmisc/psmisc#12
136+
137+Author: Christian Ehrhardt <christian.ehrhardt@canonical.com>
138+Original-Author: Craig Small <csmall@enc.com.au>
139+Origin: backport, https://gitlab.com/psmisc/psmisc/commit/258ee9166e585f87005d3a9686938a4fa26669f9
140+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1806060
141+Last-Update: 2018-12-11
142+
143+---
144+ ChangeLog | 2 +-
145+ src/killall.c | 20 ++++++++++++++------
146+ testsuite/killall.test/killall.exp | 13 +++++++++++++
147+ 3 files changed, 28 insertions(+), 7 deletions(-)
148+
149+--- a/src/killall.c
150++++ b/src/killall.c
151+@@ -856,7 +856,7 @@ main (int argc, char **argv)
152+
153+ opterr = 0;
154+ #ifdef WITH_SELINUX
155+- while ( (optc = getopt_long(argc,argv,"egy:o:ilqrs:u:vwZ:VIn:",options,NULL)) != -1) {
156++ while ( (optc = getopt_long_only(argc,argv,"egy:o:ilqrs:u:vwZ:VIn:",options,NULL)) != -1) {
157+ #else
158+ while ( (optc = getopt_long(argc,argv,"egy:o:ilqrs:u:vwVIn:",options,NULL)) != -1) {
159+ #endif
160+@@ -913,7 +913,7 @@ main (int argc, char **argv)
161+ ignore_case = 1;
162+ } else {
163+ sig_num = get_signal (argv[optind]+1, "killall");
164+- skip_error=1;
165++ skip_error=optind;
166+ }
167+ break;
168+ case 'V':
169+@@ -921,8 +921,10 @@ main (int argc, char **argv)
170+ if (strcmp(argv[optind-1],"-V") == 0 || strncmp(argv[optind-1],"--",2) == 0) {
171+ print_version();
172+ return 0;
173++ } else {
174++ sig_num = get_signal (argv[optind]+1, "killall");
175++ skip_error=optind;
176+ }
177+- sig_num = get_signal (argv[optind]+1, "killall");
178+ break;
179+ case 'n':
180+ opt_ns_pid = atoi(optarg);
181+@@ -940,6 +942,15 @@ main (int argc, char **argv)
182+ break;
183+ #endif /*WITH_SELINUX*/
184+ case '?':
185++ if (skip_error == optind)
186++ break;
187++ /* Sigh, this is a hack because -ve could be -version or
188++ * -verbose */
189++ if (strncmp(argv[optind-1], "-ve", 3) == 0) {
190++ verbose=1;
191++ exact=1;
192++ break;
193++ }
194+ /* Signal names are in uppercase, so check to see if the argv
195+ * is upper case */
196+ if (argv[optind-1][1] >= 'A' && argv[optind-1][1] <= 'Z') {
197+@@ -949,10 +960,7 @@ main (int argc, char **argv)
198+ if (argv[optind-1][1] >= '0' && argv[optind-1][1] <= '9') {
199+ sig_num = atoi(argv[optind-1]+1);
200+ } else {
201+- if (skip_error)
202+- skip_error=0;
203+- else
204+- usage(NULL);
205++ usage(NULL);
206+ }
207+ }
208+ break;
209diff --git a/debian/patches/series b/debian/patches/series
210index 4bbbe65..ad87743 100644
211--- a/debian/patches/series
212+++ b/debian/patches/series
213@@ -1 +1,4 @@
214 manpage-fixes
215+lp1806060-0001-killall-Fix-INT-option-parsing.patch
216+lp1806060-0002-killall-change-to-getopt_long.patch
217+lp1806060-0003-killall-Another-go-at-option-parsing.patch

Subscribers

People subscribed via source and target branches