Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completion for killall doesn't work #90

Open
ernstp opened this issue Nov 16, 2016 · 12 comments
Open

Completion for killall doesn't work #90

ernstp opened this issue Nov 16, 2016 · 12 comments

Comments

@ernstp
Copy link

ernstp commented Nov 16, 2016

The completion for killall has a number of issues, one of them is the linux kernel 16-byte process name limitation.
Also, processes started from a directory with a space in their name, or with a space in their process name, fail.

I haven't really looked into how to write bash completion scripts, but here's how you could get a better process name list:
cut -z -d "" -f 1 /proc/*/cmdline | xargs -0 -n1 basename

@scop
Copy link
Owner

scop commented Nov 17, 2016

The suggested approach is apparently Linux specific, so it cannot be just slapped in (the place to do this is the _pnames function in bash_completion BTW). It also has some generic portability issues: the -z argument to cut is not portable (doesn't even exist in some recentish Linux distros such as my Fedora 23), neither is -0 to xargs. And we don't want to use external utilities like basename when internal suitable mechanisms exist, such as bash's parameter expansion in this case. Search for POSIX in CONTRIBUTING.md for more info on portability.

@ThomasFaivre
Copy link

Hello,

sorry for digging this up, but I fixed this in my bashrc by redefining _killall and calling '_pnames -s' instead of just '_pnames'.

I am not familiar with Solaris & others but it seems _pnames is defined differently on those OS's so it should not do anything.

I can make the patch if you agree with that statement.

@scop
Copy link
Owner

scop commented Feb 24, 2021

@ThomasFaivre it would be good to first hear what exactly doesn't work for you, and what is your OS and version.

@ThomasFaivre
Copy link

Fair enough.

Debian 10

bash --version
GNU bash, version 5.0.3(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2019 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

bash-completion/testing,now 1:2.11-2 all [installed]

psmisc/stable,now 23.2-1 amd64 [installed,automatic]

How to reproduce:

$ <Start some process with a process name longer than 16 bytes. I'm using qemu>
$ ps -eo cmd | grep qemu
/usr/bin/qemu-system-x86_64 -k fr -daemonize --enable-kvm [...]
$ killall qemu< TAB >
$ killall -v qemu-system-x86_64
qemu-system-x86_64: no process found
$ killall -v qemu-system-x86 # manually written
Killed qemu-system-x86(31622) with signal 15

Read too fast the original post, I don't know about the space issue but I saw that pnames was reworked so that might have already been fixed. I don't know.

Using '_pnames -s' here, fixes this issue for me:

$ killall qemu< TAB >
$ killall -v qemu-system-x86

@scop
Copy link
Owner

scop commented Feb 26, 2021

I think _pnames -s "fixes" this particular case just because qemu-system-x86 happens to be short enough, and that it wouldn't work for longer (> 15 char) command basenames. Please follow the Troubleshooting section in README.md to get debug output, review it for possible information you may not want to share here, redact as appropriate, and attach the output here as an attachment.

@ThomasFaivre
Copy link

I can do that on monday, but I don't really understand. Qemu basename is qemu-system-x86_64 which is longer than 15, that is why I need to strip '_64' to make killall command happy

In [19]: len('qemu-system-x86_64')                                              
Out[19]: 18

In [20]: len('qemu-system-x86')                                                 
Out[20]: 15

I used the ps ax -o comm (== _pnames -s) to list processes and looked for truncated basenames and tried to killall one:

$ killall -vi at-spi2-registryd 
at-spi2-registryd: no process found
$ killall -vi at-spi2-registr
Kill at-spi2-registr(28590) ? (y/N) n
at-spi2-registr: no process found <<< This is due to the 'n' answer to the interactive option

Am I missing something here?

@scop
Copy link
Owner

scop commented Mar 1, 2021

Oh, I missed the x86 vs x86_64 difference. But for me on Ubuntu 18.04, killall seems to work with the complete process names, not abbreviations, so I believe switching to _pnames -s would break the completion which seems to work fine as it is. psmisc is 23.1-1ubuntu0.1.

$ ps wax | grep [g]sd-print-noti
 2203 tty1     Sl+    0:00 /usr/lib/gnome-settings-daemon/gsd-print-notifications
 3973 tty2     Sl+    0:00 /usr/lib/gnome-settings-daemon/gsd-print-notifications
$ ps axo comm= | grep [g]sd-print-noti
gsd-print-notif
gsd-print-notif
$ killall -s 0 gsd-print-notif
gsd-print-notif: no process found
$ killall -s 0 gsd-print-notifications
$ echo $?
0

@scop
Copy link
Owner

scop commented Mar 1, 2021

Also:

$ killall -vi at-spi2-registryd 
Kill at-spi2-registr(3595) ? (y/N) n
at-spi2-registryd: no process found  <<< also due to the 'n' choice
$ killall -vi at-spi2-registr
at-spi2-registr: no process found

@ThomasFaivre
Copy link

It seems it was fixed in killall:

https://gitlab.com/psmisc/psmisc/-/blob/master/ChangeLog#L19

It was backported to ubuntu 18.04, and is present in Ubuntu 20.04 and Debian testing (future 11)

I can live with the workaround in my bashrc (or I could upgrade to testing version). Do we still want to do something here? Probably not useful since Debian 10 wil probably not ship the fix we make...

@scop
Copy link
Owner

scop commented Mar 1, 2021

Ok, confused here. psmisc 23.2 according to the referred changelog entry is supposed to work with the longer process names ("Command names increased from 16 to 64 characters" I guess), right? Your report says you have 23.2 but it actually doesn't work with the long ones, but only the truncated ones?

@ThomasFaivre
Copy link

Hmmm that's very true.

Let's try that again:

I believe the issue was actually introduced in 23.2 with this commit (and a fix):

https://gitlab.com/psmisc/psmisc/-/commit/1e2f38a202798a78554ae5f5d12f697f3607f89f
https://gitlab.com/psmisc/psmisc/-/commit/acdcbf30c68053a25a1a5b5a6604a8382912be71

This was done to handle the new comm length for "new kernel". Modification I still can't understand since kernel 5.12-rc1 still uses 16 as TASK_COMM_LEN:

https://github.com/torvalds/linux/blob/master/include/linux/sched.h#L215
https://gitlab.com/psmisc/psmisc/-/blob/master/src/comm.h#L30

And someone got as confused as me:

https://gitlab.com/psmisc/psmisc/-/issues/19

But anyway, it was fixed in 23.3:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=912748 <<< this is actually the issue I'm seeing. Found in changelog for 23.3
https://gitlab.com/psmisc/psmisc/commit/1188315cd037d73bf946a0003b70c6423cc330d2 <<< note that there is a typo in changelog
https://gitlab.com/psmisc/psmisc/-/commit/2209bc12fedd75aca8b79801a842b35f87f9b727 <<< fixes the typo

To summarize:

Ubuntu-18.04: Version is 23.1-1ubuntu0.1 - does not handle the new comm length of 64 => No issue
Ubuntu-20.04: Version is 23.3-1 - handle new (64) and old (16) comm length => No issue
Debian-10: Version is 23.2-1 - handle new (64) comm length but not the old one => Issue
Debian-11: Version will be at least 23.3-1 - handle new (64) and old (16) comm length => No issue

So yeah, I don't think anything should change here. I have contacted the maintainer for the psmisc package about releasing the fix in Debian 10, but maybe a debian patch should be added to the bash-completion package in (and only in) Debian 10 to handle that specific issue. It will depend on the answer from the maintainer.

Sorry for the noise!

@scop
Copy link
Owner

scop commented Mar 1, 2021

No problem, thanks for the thorough detective work. Guess we should keep this open anyway as I gather at least some of the originally reported issues still do persist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants