bug#29156: 25.3; eshell/kill does not understand -<signal>

bug#29156: 25.3; eshell/kill does not understand -<signal>

[Pierre Neidhardt]

- emacs -Q
- M-x eshell

~ $ kill -9 emacs
kill: bad pid: -9


In GNU Emacs 25.3.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.16)
 of 2017-11-05 built on dhiov23k
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description:	Gentoo Base System release 2.4.1

Configured using:
 'configure --prefix=/usr --build=x86_64-pc-linux-gnu
 --host=x86_64-pc-linux-gnu --mandir=/usr/share/man
 --infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc
 --localstatedir=/var/lib --disable-dependency-tracking
 --disable-silent-rules --docdir=/usr/share/doc/emacs-25.3
 --htmldir=/usr/share/doc/emacs-25.3/html --libdir=/usr/lib64
 --program-suffix=-emacs-25 --infodir=/usr/share/info/emacs-25
 --localstatedir=/var
 --enable-locallisppath=/etc/emacs:/usr/share/emacs/site-lisp
 --with-gameuser=:gamestat --without-compress-install
 --with-file-notification=inotify --enable-acl --without-dbus
 --without-modules --without-gpm --without-hesiod --without-kerberos
 --without-kerberos5 --with-xml2 --without-selinux --with-gnutls
 --without-wide-int --with-zlib --with-sound=alsa --with-x --without-ns
 --without-gconf --without-gsettings --without-toolkit-scroll-bars
 --with-gif --with-jpeg --with-png --with-rsvg --with-tiff --with-xpm
 --with-imagemagick --with-xft --without-cairo --without-libotf
 --without-m17n-flt --with-x-toolkit=gtk3 --without-xwidgets
 GENTOO_PACKAGE=app-editors/emacs-25.3 'CFLAGS=-march=ivybridge -O2
 -pipe' CPPFLAGS= 'LDFLAGS=-Wl,-O1 -Wl,--as-needed''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND NOTIFY ACL GNUTLS LIBXML2
FREETYPE XFT ZLIB GTK3 X11

Important settings:
  value of $LANG: en_US.utf8
  locale-coding-system: utf-8-unix

bug#29156: 25.3; eshell/kill does not understand -<signal>

[Noam Postavsky]

tags 29156 + confirmed
quit

Pierre Neidhardt <ambrevar@gmail.com> writes:

> - emacs -Q
> - M-x eshell
>
> ~ $ kill -9 emacs
> kill: bad pid: -9

This bit converts the "-9" into an integer:

(defun eshell-lisp-command (object &optional args)
   ...
		  ;; if any of the arguments are flagged as numbers
		  ;; waiting for conversion, convert them now
		  (unless (get object 'eshell-no-numeric-conversions)
		    (while args
		      (let ((arg (car args)))
			(if (and (stringp arg)
				 (> (length arg) 0)
				 (not (text-property-not-all
				       0 (length arg) 'number t arg)))
			    (setcar args (string-to-number arg))))
		      (setq args (cdr args))))
  ...

Whereas this bit expects "-9" as a string:

(defun eshell/kill (&rest args)
  ...
      (when (stringp arg)
        (cond
         ((string-match "\\`-[[:digit:]]+\\'" arg)
          (setq signum (abs (string-to-number arg))))
         ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
          (setq signum (abs (string-to-number arg)))))
  ...

Using "-SIGKILL" doesn't work either, because eshell/kill calls
`string-to-number' on it, giving 0.  Apparently nobody ever actually
used this feature before.

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 22 bytes --]

[forwarding to list]


[-- Attachment #2: Type: message/rfc822, Size: 2025 bytes --]

From: Eric Skoglund <eric@pagefault.se>
To: Noam Postavsky <npostavs@users.sourceforge.net>
Subject: Re: bug#29156: 25.3; eshell/kill does not understand -<signal>
Date: Fri, 16 Mar 2018 15:01:32 +0100
Message-ID: <87fu50ktkz.fsf@pagefault.se>

--=-=-=
Content-Type: text/plain


First time contributor. Here's a patch that allows eshell/kill to handle
both the -9 case and the -SIGKILL case.

--=-=-=
Content-Type: text/x-patch
Content-Disposition: inline; filename=0001-Make-eshell-kill-handle-signal-and-SIGNALNAME.patch
Content-Description: eshell/kill patch

From 2789c82b27cfac175d6d04260db78a54a2f26b01 Mon Sep 17 00:00:00 2001
From: Eric Skoglund <eric@pagefault.se>
Date: Fri, 16 Mar 2018 14:49:56 +0100
Subject: [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

     * lisp/eshell/esh-proc.el: Handle -<signal> and -<SIGNALNAME>
---
 lisp/eshell/esh-proc.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index b3bd7a7245..6dab6636b0 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -178,12 +178,12 @@ eshell/kill
          ((string-match "\\`-[[:digit:]]+\\'" arg)
           (setq signum (abs (string-to-number arg))))
          ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
-          (setq signum (abs (string-to-number arg)))))
+          (setq signum (make-symbol (substring arg 1 (length arg))))))
         (setq args (cdr args))))
     (while args
       (let ((arg (if (eshell-processp (car args))
                      (process-id (car args))
-                   (car args))))
+                   (string-to-number (car args)))))
         (when arg
           (cond
            ((null arg)
-- 
2.13.6


--=-=-=--

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Noam Postavsky]

> From: Eric Skoglund <eric@pagefault.se>

> First time contributor. Here's a patch that allows eshell/kill to handle
> both the -9 case and the -SIGKILL case.

Thanks!

>      * lisp/eshell/esh-proc.el: Handle -<signal> and -<SIGNALNAME>

Minor formatting nitpick, you should have the function name here:

    * lisp/eshell/esh-proc.el (eshell-kill): ...

More importantly, could you explain a bit how your change works/why it's
correct?

>           ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
> -          (setq signum (abs (string-to-number arg)))))
> +          (setq signum (make-symbol (substring arg 1 (length arg))))))

Not sure this `make-symbol' call, should it rather be `intern'?  (Maybe
we should update signal-process take a string as well a symbol.)

>          (setq args (cdr args))))
>      (while args
>        (let ((arg (if (eshell-processp (car args))
>                       (process-id (car args))
> -                   (car args))))
> +                   (string-to-number (car args)))))

I think the args have already been converted to numbers, or did you mean
to also add a (put 'eshell/kill 'eshell-no-numeric-conversions t)?

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Date: Fri, 16 Mar 2018 20:34:25 -0400
> Cc: Eric Skoglund <eric@pagefault.se>, Pierre Neidhardt <ambrevar@gmail.com>
> 
> >           ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
> > -          (setq signum (abs (string-to-number arg)))))
> > +          (setq signum (make-symbol (substring arg 1 (length arg))))))
> 
> Not sure this `make-symbol' call, should it rather be `intern'?

Yes, I think intern is better here.

> (Maybe we should update signal-process take a string as well a
> symbol.)

Possibly.

Btw, the doc string of eshell/kill should be updated to reflect the
fact we now support symbolic names of Unix signals.  Also, NEWS and
the Eshell manual should be updated.

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 82 bytes --]

[forwarding to list, please use "Reply All" to keep 29156@debbugs.gnu.org on Cc]


[-- Attachment #2: Type: message/rfc822, Size: 1183 bytes --]

From: Eric Skoglund <eric@pagefault.se>
To: Noam Postavsky <npostavs@gmail.com>
Subject: Re: bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>
Date: Sat, 17 Mar 2018 15:11:41 +0100
Message-ID: <877eqa7pwi.fsf@pagefault.se>


Thanks for the review!

> More importantly, could you explain a bit how your change works/why it's
> correct?

I'm guessing you mean in the commit message and not here in the bug
report right?

> Not sure this `make-symbol' call, should it rather be `intern'?  (Maybe
> we should update signal-process take a string as well a symbol.)

Saw that Eli also agreed, for future reference when do we want to use
`make-symbol` vs `intern`?

> I think the args have already been converted to numbers, or did you mean
> to also add a (put 'eshell/kill 'eshell-no-numeric-conversions t)?

Yes indeed. I'm unsure how I managed to lose that in git...

// Eric

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Noam Postavsky]

> From: Eric Skoglund <eric@pagefault.se>

>> More importantly, could you explain a bit how your change works/why it's
>> correct?
>
> I'm guessing you mean in the commit message and not here in the bug
> report right?

Either, both :)

(If you send a new patch with the explanation in the commit message,
it's automatically in the bug report as well anyway)

>> Not sure [about] this `make-symbol' call, should it rather be
>> `intern'?  (Maybe we should update signal-process take a string as
>> well a symbol.)
>
> Saw that Eli also agreed, for future reference when do we want to use
> `make-symbol` vs `intern`?

Usually, `make-symbol' is only for macros, where you want a symbol that
is not `eq' to any other.  Sometimes it's handy for making a unique
object at run-time.  In this case, since we only care about the symbol
name, it doesn't really matter, it's just a bit surprising to see it
because it's usually a mistake in a non-macro context.

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Eric Skoglund]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Fri, 16 Mar 2018 20:34:25 -0400
>> Cc: Eric Skoglund <eric@pagefault.se>, Pierre Neidhardt <ambrevar@gmail.com>
>> 
>> >           ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
>> > -          (setq signum (abs (string-to-number arg)))))
>> > +          (setq signum (make-symbol (substring arg 1 (length arg))))))
>> 
>> Not sure this `make-symbol' call, should it rather be `intern'?
>
> Yes, I think intern is better here.
>
>> (Maybe we should update signal-process take a string as well a
>> symbol.)
>
> Possibly.
>
> Btw, the doc string of eshell/kill should be updated to reflect the
> fact we now support symbolic names of Unix signals.  Also, NEWS and
> the Eshell manual should be updated.

Here is an updated patch which hopefully fixes all the issues with the
previous.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch v2 --]
[-- Type: text/x-patch, Size: 2966 bytes --]

From 54bca6f5420d80abeaf9c1da5b73d50112eaab71 Mon Sep 17 00:00:00 2001
From: Eric Skoglund <eric@pagefault.se>
Date: Fri, 16 Mar 2018 14:49:56 +0100
Subject: [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

     * lisp/eshell/esh-proc.el (eshell/kill):
     Handle the argument parsing and numeric conversion in function
     in order to parse -signal and -SIGNALNAME correctly.
---
 doc/misc/eshell.texi    | 2 +-
 etc/NEWS                | 5 +++++
 lisp/eshell/esh-proc.el | 9 ++++++---
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 80077e5ccd..bda6159488 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -330,7 +330,7 @@ Built-ins
 @item kill
 @cmindex kill
 Kill processes.  Takes a PID or a process object and an optional
-signal specifier.
+signal specifier which can either be a number or a signal name.
 
 @item listify
 @cmindex listify
diff --git a/etc/NEWS b/etc/NEWS
index b6c4157384..bfd9a33040 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -273,6 +273,11 @@ To restore the old behavior, use
 *** The function 'shell-uniquify-list' has been renamed from
 'eshell-uniqify-list'.
 
+*** The function eshell/kill is now able to handle signal switches.
+Previously eshell/kill would fail if provided a kill signal to send to the
+process. It now handles both numeric signals and the string version of
+the signals.
+
 ** Pcomplete
 *** The function 'pcomplete-uniquify-list' has been renamed from
 'pcomplete-uniqify-list'.
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index b3bd7a7245..1fe4b324a6 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -167,7 +167,8 @@ eshell/jobs
 (defun eshell/kill (&rest args)
   "Kill processes.
 Usage: kill [-<signal>] <pid>|<process> ...
-Accepts PIDs and process objects."
+Accepts PIDs and process objects.  Optionally accept signals
+and signal names."
   ;; If the first argument starts with a dash, treat it as the signal
   ;; specifier.
   (let ((signum 'SIGINT))
@@ -178,12 +179,12 @@ eshell/kill
          ((string-match "\\`-[[:digit:]]+\\'" arg)
           (setq signum (abs (string-to-number arg))))
          ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
-          (setq signum (abs (string-to-number arg)))))
+          (setq signum (intern (substring arg 1 (length arg))))))
         (setq args (cdr args))))
     (while args
       (let ((arg (if (eshell-processp (car args))
                      (process-id (car args))
-                   (car args))))
+                   (string-to-number (car args)))))
         (when arg
           (cond
            ((null arg)
@@ -198,6 +199,8 @@ eshell/kill
       (setq args (cdr args))))
   nil)
 
+(put 'eshell/kill 'eshell-no-numeric-conversions t)
+
 (defun eshell-read-process-name (prompt)
   "Read the name of a process from the minibuffer, using completion.
 The prompt will be set to PROMPT."
-- 
2.13.6


[-- Attachment #3: Type: text/plain, Size: 10 bytes --]



// Eric

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Eli Zaretskii]

> From: Eric Skoglund <eric@pagefault.se>
> Cc: Noam Postavsky <npostavs@gmail.com>, 29156@debbugs.gnu.org,  ambrevar@gmail.com
> Date: Sat, 17 Mar 2018 18:43:29 +0100
> 
>      * lisp/eshell/esh-proc.el (eshell/kill):
>      Handle the argument parsing and numeric conversion in function
>      in order to parse -signal and -SIGNALNAME correctly.
> ---
>  doc/misc/eshell.texi    | 2 +-
>  etc/NEWS                | 5 +++++
>  lisp/eshell/esh-proc.el | 9 ++++++---
>  3 files changed, 12 insertions(+), 4 deletions(-)

The commit log should mention changes to all the files you are
changing.

> +*** The function eshell/kill is now able to handle signal switches.
> +Previously eshell/kill would fail if provided a kill signal to send to the
> +process. It now handles both numeric signals and the string version of
          ^^
Two spaces between sentences, please.  Also, I'd rephrase the last
sentence:

  It now accepts signals specified either by name or by its number.

Thanks.

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Noam Postavsky]

Thanks, do you have copyright assignment for Emacs? (the change is small
enough to install regardless, I'm asking just to know if it should be
marked as a tiny change).

A couple more minor nitpicks:

>      * doc/misc/eshell.texi (kill): Update docs to reflect new
>      function behaviour
>      * etc/NEWS: Mention new eshell/kill behaviour

Missing periods here.

> +          (setq signum (intern (substring arg 1 (length arg))))))

That could be just (substring arg 1)

P.S. don't worry about the forwarding thing, it happens often enough
that I've written an Emacs command to mostly automate it.

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Eric Skoglund]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

Noam Postavsky <npostavs@gmail.com> writes:

> Thanks, do you have copyright assignment for Emacs? (the change is small
> enough to install regardless, I'm asking just to know if it should be
> marked as a tiny change).

Yes it was completed on 2016-12-05 when I had grand plans to contribute
to emacs (maybe thoose plans can be fullfilled now :)).

>
> A couple more minor nitpicks:
>
>>      * doc/misc/eshell.texi (kill): Update docs to reflect new
>>      function behaviour
>>      * etc/NEWS: Mention new eshell/kill behaviour
>
> Missing periods here.
>
>> +          (setq signum (intern (substring arg 1 (length arg))))))
>
> That could be just (substring arg 1)
>

Thanks!

> P.S. don't worry about the forwarding thing, it happens often enough
> that I've written an Emacs command to mostly automate it.

Ah great :)

Here is the latest patch version. Thanks for all the help for a first
time contributor!


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch v4 --]
[-- Type: text/x-patch, Size: 3086 bytes --]

From 7588028d623a8312a7a359f2be4ba84426d0619e Mon Sep 17 00:00:00 2001
From: Eric Skoglund <eric@pagefault.se>
Date: Fri, 16 Mar 2018 14:49:56 +0100
Subject: [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

     * lisp/eshell/esh-proc.el (eshell/kill):
     Handle the argument parsing and numeric conversion in function
     in order to parse -signal and -SIGNALNAME correctly.
     * doc/misc/eshell.texi (kill): Update docs to reflect new
     function behaviour.
     * etc/NEWS: Mention new eshell/kill behaviour.
---
 doc/misc/eshell.texi    | 2 +-
 etc/NEWS                | 5 +++++
 lisp/eshell/esh-proc.el | 9 ++++++---
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 80077e5ccd..bda6159488 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -330,7 +330,7 @@ Built-ins
 @item kill
 @cmindex kill
 Kill processes.  Takes a PID or a process object and an optional
-signal specifier.
+signal specifier which can either be a number or a signal name.
 
 @item listify
 @cmindex listify
diff --git a/etc/NEWS b/etc/NEWS
index b6c4157384..7df6eb4387 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -273,6 +273,11 @@ To restore the old behavior, use
 *** The function 'shell-uniquify-list' has been renamed from
 'eshell-uniqify-list'.
 
+*** The function eshell/kill is now able to handle signal switches.
+Previously eshell/kill would fail if provided a kill signal to send to the
+process.  It now accepts signals specified either by name or by its number.
+
+
 ** Pcomplete
 *** The function 'pcomplete-uniquify-list' has been renamed from
 'pcomplete-uniqify-list'.
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index b3bd7a7245..a7855d81db 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -167,7 +167,8 @@ eshell/jobs
 (defun eshell/kill (&rest args)
   "Kill processes.
 Usage: kill [-<signal>] <pid>|<process> ...
-Accepts PIDs and process objects."
+Accepts PIDs and process objects.  Optionally accept signals
+and signal names."
   ;; If the first argument starts with a dash, treat it as the signal
   ;; specifier.
   (let ((signum 'SIGINT))
@@ -178,12 +179,12 @@ eshell/kill
          ((string-match "\\`-[[:digit:]]+\\'" arg)
           (setq signum (abs (string-to-number arg))))
          ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
-          (setq signum (abs (string-to-number arg)))))
+          (setq signum (intern (substring arg 1)))))
         (setq args (cdr args))))
     (while args
       (let ((arg (if (eshell-processp (car args))
                      (process-id (car args))
-                   (car args))))
+                   (string-to-number (car args)))))
         (when arg
           (cond
            ((null arg)
@@ -198,6 +199,8 @@ eshell/kill
       (setq args (cdr args))))
   nil)
 
+(put 'eshell/kill 'eshell-no-numeric-conversions t)
+
 (defun eshell-read-process-name (prompt)
   "Read the name of a process from the minibuffer, using completion.
 The prompt will be set to PROMPT."
-- 
2.13.6

bug#29156: 25.3; eshell/kill does not understand -<signal>, [PATCH] Make eshell/kill handle -<signal> and -<SIGNALNAME>

[Noam Postavsky]

tags 29156 fixed
close 29156 27.1
quit

Eric Skoglund <eric@pagefault.se> writes:

> Noam Postavsky <npostavs@gmail.com> writes:
>
>> Thanks, do you have copyright assignment for Emacs?

> Yes it was completed on 2016-12-05 when I had grand plans to contribute
> to emacs (maybe thoose plans can be fullfilled now :)).

Plans fullfilled :) [1: 1be6a21fd8] 

> Here is the latest patch version. Thanks for all the help for a first
> time contributor!

Thanks for your contribution!

[1: 1be6a21fd8]: 2018-03-25 11:20:20 -0400
  Make eshell/kill handle -<signal> and -<SIGNALNAME> (Bug#29156)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=1be6a21fd8b5ade67f7f69f964331aa570623683