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

Proposed by Christian Ehrhardt  on 2018-12-11
Status: Merged
Approved by: Christian Ehrhardt  on 2018-12-14
Approved revision: 0028bb2bbb245d4dc7709b8d9c531ba07ea90180
Merged at revision: 0028bb2bbb245d4dc7709b8d9c531ba07ea90180
Proposed branch: ~paelzer/ubuntu/+source/psmisc:lp1806060-fix-killall-option-parsing-cosmic
Merge into: ubuntu/+source/psmisc:ubuntu/cosmic-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+360686@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 ...

Andreas Hasenack (ahasenack) wrote :

range-diff is nice for SRUs :)

git range-diff pkg/ubuntu/bionic-devel..lp1806060-fix-killall-option-parsing-bionic pkg/ubuntu/cosmic-devel..lp1806060-fix-killall-option-parsing-cosmic
1: f4614b0 = 1: ee79ceb d/patches/lp1806060-*: fix killall option parsing (LP: #1806060)
2: fbe4683 < -: ------- changelog: fix killall option parsing (LP: #1806060)
-: ------- > 2: 0028bb2 changelog: fix killall option parsing (LP: #1806060)

:)

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 20cf8fd..8f784c8 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+psmisc (23.1-1ubuntu1.1) cosmic; 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-1build1) cosmic; urgency=medium
16
17 * No-change rebuild for ncurses soname changes.
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