small problem with kill --queue

small problem with kill --queue

[Benno Schulenberg]

Hello Sami,

./kill --queue 11 12
kill: sending signal to 12 failed: Operation not permitted

/kill --queue data 12
kill: unknown signal data; valid signals:
 1 HUP      2 INT      3 QUIT     4 ILL      5 TRAP     [...]

But "data" is not a signal, it is meant to be a number, an integer,
a value passed to the signalled process.

In misc-utils.c it does this:

        arg = *argv;
        if ((ctl->numsig = arg_to_signum(arg, 0)) < 0)
                err_nosig(arg);

But it should not run arg_to_signum() on the argument, it
should simply parse it as an integer, and when that fails,
it should not call err_nosig() but say something like "not
a number".  Right?

Benno

-- 
http://www.fastmail.fm - mmm... Fastmail...

Re: small problem with kill --queue

[Sami Kerola]

On 7 July 2014 21:34, Benno Schulenberg <bensberg@justemail.net> wrote:
> ./kill --queue 11 12
> kill: sending signal to 12 failed: Operation not permitted
>
> /kill --queue data 12
> kill: unknown signal data; valid signals:
>  1 HUP      2 INT      3 QUIT     4 ILL      5 TRAP     [...]
>
> But "data" is not a signal, it is meant to be a number, an integer,
> a value passed to the signalled process.
>
> In misc-utils.c it does this:
>
>         arg = *argv;
>         if ((ctl->numsig = arg_to_signum(arg, 0)) < 0)
>                 err_nosig(arg);
>
> But it should not run arg_to_signum() on the argument, it
> should simply parse it as an integer, and when that fails,
> it should not call err_nosig() but say something like "not
> a number".  Right?

Hi Benno,

Right. I am not sure did I mess up or someone else earlier when
--queue was introduced. Which way ever that was now when I look
sigqueue(3) manual page your change proposal makes sense. Please have
a look of the change below, that I hope to get reviewed before the
final v2.25 is out.

--->8----
From: Sami Kerola <kerolasa@iki.fi>
Date: Mon, 7 Jul 2014 23:31:56 +0100
Subject: [PATCH] kill: use --queue argument as sigval integer

The sigqueue(3) takes two values, signal and sigval.  Contents of the
signal can be altered with --signal option argument, so the --queue
argument should be reserved to affect sigval_int.

Reference: http://man7.org/linux/man-pages/man3/sigqueue.3.html
Reported-by: Benno Schulenberg <bensberg@justemail.net>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/kill.1     | 18 ++++++++++++------
 misc-utils/kill.c     |  4 +---
 tests/ts/kill/options |  4 ++--
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/misc-utils/kill.1 b/misc-utils/kill.1
index dd1b1ee..65d094f 100644
--- a/misc-utils/kill.1
+++ b/misc-utils/kill.1
@@ -98,14 +98,20 @@ This functionality is deprecated, and will not be
removed in March 2016.
 Use
 .BR sigqueue (2)
 rather than
-.BR kill (2)
-and the
+.BR kill (2).
+Argument
 .I sigval
-argument is used to specify an integer to be sent with the signal.  If the
-receiving process has installed a handler for this signal using the SA_SIGINFO
-flag to
+is an integer that sent with the signal.  If the receiving process has
+installed a handler for this signal using the SA_SIGINFO flag to
 .BR sigaction (2),
-then it can obtain this data via the si_value field of the siginfo_t structure.
+then it can obtain the sigval integer via the si_value field of the
+siginfo_t structure passed as the second argument to the handler.
+The
+.B \-\-queue
+should be combined with
+.BI \-\-signal " signal"
+when other than a SIGTERM is wanted to accompanied with
+.IR sigval .
 .SH NOTES
 It is not possible to send a signal to explicitly selected thread in a
 multithreaded process by
diff --git a/misc-utils/kill.c b/misc-utils/kill.c
index 5d2f27d..a24087f 100644
--- a/misc-utils/kill.c
+++ b/misc-utils/kill.c
@@ -412,9 +412,7 @@ static char **parse_arguments(int argc, char
**argv, struct kill_control *ctl)
  errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
  argc--, argv++;
  arg = *argv;
- if ((ctl->numsig = arg_to_signum(arg, 0)) < 0)
- err_nosig(arg);
- ctl->sigdata.sival_int = ctl->numsig;
+ ctl->sigdata.sival_int = strtos32_or_err(arg, _("argument error"));
  ctl->use_sigval = 1;
  continue;
  }
diff --git a/tests/ts/kill/options b/tests/ts/kill/options
index 6cdaa24..5af78d9 100755
--- a/tests/ts/kill/options
+++ b/tests/ts/kill/options
@@ -51,8 +51,8 @@ try_option -s 1
 try_option --signal 1
 try_option --signal HUP
 try_option --signal SIGHUP
-try_option -q HUP
-try_option --queue HUP
+try_option -s 1 -q 42
+try_option -s 1 --queue 42
 try_option -1
 try_option -HUP
 try_option -SIGHUP
-- 
2.0.1

Re: small problem with kill --queue

[Benno Schulenberg]

Hi Sami,

On Tue, Jul 8, 2014, at 00:39, Sami Kerola wrote:
> Right. I am not sure did I mess up or someone else earlier when
> --queue was introduced.

It looks like you made the mistaken change four months ago,
in commit 9e8dffd5cd29f03029b1ac99eecb129532ca5c0f.

> Which way ever that was now when I look
> sigqueue(3) manual page your change proposal makes sense. Please have
> a look of the change below, that I hope to get reviewed before the
> final v2.25 is out. [...]
>
> +The
> +.B \-\-queue
> +should be combined with
> +.BI \-\-signal " signal"
> +when other than a SIGTERM is wanted to accompanied with
> +.IR sigval .

I don't think it's necessary to explain again that wanting to send any
other signal than TERM requires the --signal option; this is already at
the top of the page.

Also, thinking again, this word "sigval" looks far too similar to "signal",
it would be much clearer to use the plain word "value" instead.  So my
upcoming patches for the usage text and man page use that instead.

> + ctl->sigdata.sival_int = strtos32_or_err(arg, _("argument error"));

Maybe "argument of --queue must be an integer" instead?

My patch to kill.c also improves the error message when the
signal name or number cannot be interpreted.  Currently:

./kill --sigmal HUP foobar
kill: invalid sigval argument

Huh?

New:

./kill --sigmal HUP foobar
kill: invalid signal name or number: -sigmal

Ah, typo.

Benno

-- 
http://www.fastmail.fm - Access all of your messages and folders
                          wherever you are

Re: small problem with kill --queue

[Sami Kerola]

On 9 July 2014 14:29, Benno Schulenberg <bensberg@justemail.net> wrote:
> On Tue, Jul 8, 2014, at 00:39, Sami Kerola wrote:
>> Right. I am not sure did I mess up or someone else earlier when
>> --queue was introduced.
>
> It looks like you made the mistaken change four months ago,
> in commit 9e8dffd5cd29f03029b1ac99eecb129532ca5c0f.

Yep. Another bug I created. Oops & sorry.

>> Which way ever that was now when I look
>> sigqueue(3) manual page your change proposal makes sense. Please have
>> a look of the change below, that I hope to get reviewed before the
>> final v2.25 is out. [...]
>>
>> +The
>> +.B \-\-queue
>> +should be combined with
>> +.BI \-\-signal " signal"
>> +when other than a SIGTERM is wanted to accompanied with
>> +.IR sigval .
>
> I don't think it's necessary to explain again that wanting to send any
> other signal than TERM requires the --signal option; this is already at
> the top of the page.
>
> Also, thinking again, this word "sigval" looks far too similar to "signal",
> it would be much clearer to use the plain word "value" instead.  So my
> upcoming patches for the usage text and man page use that instead.

I tried to think possible person who is trying to write program(s) that
would use the kill --queue mechanism.  Maybe it is good enoug for such
persons to find this email conversation from web archive, with remark the
--queue <value> is the same value as in

sigaction(3p) SA_SIGINFO void func(int signo, siginfo_t *info, void *context);

the 'si_value' value in 'siginfo_t' struture,  and in sigqueue(3)

int sigqueue(pid_t pid, int sig, const union sigval value);

the 'int sival_int' in 'union sigval'.

That said, the following patch looks good to me.

[PATCH 2/2] docs: fix many wording and some formatting issues in the man page of kill

>> + ctl->sigdata.sival_int = strtos32_or_err(arg, _("argument error"));
>
> Maybe "argument of --queue must be an integer" instead?
>
> My patch to kill.c also improves the error message when the
> signal name or number cannot be interpreted.  Currently:
>
> ./kill --sigmal HUP foobar
> kill: invalid sigval argument
>
> Huh?
>
> New:
>
> ./kill --sigmal HUP foobar
> kill: invalid signal name or number: -sigmal

You took care of this already.

[PATCH 1/2] textual: fix the usage message of kill

Incoming change from me[*] is intented to be applied on top of the
textual fix.

[*] [PATCH] kill: use --queue option argument as sigval integer value

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

[PATCH] kill: use --queue option argument as sigval integer value

[Sami Kerola]

The sigqueue(3) takes two values, signal and sigval.  Contents of the
signal can be altered with --signal option argument, so the --queue
argument should be reserved to affect sigval_int.

This is regression fix introduced by commit
9e8dffd5cd29f03029b1ac99eecb129532ca5c0f.

Reference: http://man7.org/linux/man-pages/man3/sigqueue.3.html
Reported-by: Benno Schulenberg <bensberg@justemail.net>
Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 misc-utils/kill.c     | 4 +---
 tests/ts/kill/options | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/misc-utils/kill.c b/misc-utils/kill.c
index f2362c3..2f89427 100644
--- a/misc-utils/kill.c
+++ b/misc-utils/kill.c
@@ -412,9 +412,7 @@ static char **parse_arguments(int argc, char **argv, struct kill_control *ctl)
 				errx(EXIT_FAILURE, _("%s and %s are mutually exclusive"), "--pid", "--queue");
 			argc--, argv++;
 			arg = *argv;
-			if ((ctl->numsig = arg_to_signum(arg, 0)) < 0)
-				err_nosig(arg);
-			ctl->sigdata.sival_int = ctl->numsig;
+			ctl->sigdata.sival_int = strtos32_or_err(arg, _("argument error"));
 			ctl->use_sigval = 1;
 			continue;
 		}
diff --git a/tests/ts/kill/options b/tests/ts/kill/options
index 6cdaa24..5af78d9 100755
--- a/tests/ts/kill/options
+++ b/tests/ts/kill/options
@@ -51,8 +51,8 @@ try_option -s 1
 try_option --signal 1
 try_option --signal HUP
 try_option --signal SIGHUP
-try_option -q HUP
-try_option --queue HUP
+try_option -s 1 -q 42
+try_option -s 1 --queue 42
 try_option -1
 try_option -HUP
 try_option -SIGHUP
-- 
2.0.1

Re: [PATCH] kill: use --queue option argument as sigval integer value

[Karel Zak]

On Wed, Jul 09, 2014 at 09:49:03PM +0100, Sami Kerola wrote:
>  misc-utils/kill.c     | 4 +---
>  tests/ts/kill/options | 4 ++--
>  2 files changed, 3 insertions(+), 5 deletions(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com