On 25/01/2024 19:52, Grisha Levit wrote:
On Thu, Jan 25, 2024, 09:50 Pádraig Brady <[email protected]> wrote:
This mostly looks good, except:
- No need to clear the errno before kill(3).
- Better to use SIG%d rather than the bare %d for signal _names_, as we already
parse this format
Makes sense, done below.
* src/operand2sig.c (operand2sig): Drop signame argument, accept all
signal numbers <= SIGNUM_BOUND. All callers updated.
* src/env.c (parse_signal_action_params, reset_signal_handlers)
(parse_block_signal_params, set_signal_proc_mask)
(list_signal_handling): Accept all signal numbers <= SIGNUM_BOUND,
use SIG%d for printing if necessary.
* src/kill.c (list_signals, main): Likewise.
(send_signals): Check errno from kill(3) for bad signo.
* src/timeout.c (main): Update operand2sig call.
* tests/misc/kill.sh: Test listing all signal numbers.
I've made a few adjustments:
- Clarified the commit message.
- I've gone back to have kill -l produce a bare number for unnamed signals,
because:
this is consistent with util-linux,
SIG%d is only parseable by coreutils (not util-linux or bash),
easier to programatically determine if a name is defined.
I've left as SIG%d for -t output as that's a coreutils specific option,
and not really programatically consumable anyway.
- I've added a validation check for `env --block=32` so that it fails
like `env --default=32` and `env --ignore=32`.
I.e. exits with EXIT_CANCELED.
- Added a NEWS entry.
I'll apply this later.
thanks!
Pádraig
From b8d1b00e21a86270fbbe5903a41319894db266df Mon Sep 17 00:00:00 2001
From: Grisha Levit <[email protected]>
Date: Thu, 25 Jan 2024 14:52:50 -0500
Subject: [PATCH] env,kill,timeout: support unnamed signals
Some signals with values less that the max signal number for the system
do not have defined names. For example, currently on amd64 Linux,
signals 32 and 33 do not have defined names, and Android has a wider
gap of undefined names where it reserves some realtime signals.
Previously the signal listing in env ended up reusing the name
of the last printed valid signal (the repeated HUP below):
$ env --list-signal-handling true
HUP ( 1): IGNORE
HUP (32): BLOCK
HUP (38): IGNORE
..and the corresponding signal numbers were rejected as operands for the
env, kill, and timeout commands.
This patch removes the requirement that sig2str returns 0 for a signal
number associated with an operand. This allows unnamed signals to be in
the sets `env' attempts to manipulate when a --*-signal option is used
with no argument, and kill(1) and timeout(1) to send such unnamed
signals.
* src/operand2sig.c (operand2sig): Drop signame argument, accept all
signal numbers <= SIGNUM_BOUND. All callers updated.
* src/env.c (parse_signal_action_params, reset_signal_handlers)
(parse_block_signal_params, set_signal_proc_mask)
(list_signal_handling): Accept all signal numbers <= SIGNUM_BOUND,
use SIG%d for printing if necessary.
* src/kill.c (list_signals, main): Likewise.
(send_signals): Check errno from kill(3) for bad signo.
* src/timeout.c (main): Update operand2sig call.
* tests/misc/kill.sh: Test listing all signal numbers.
* NEWS: Mention the improvement.
---
NEWS | 3 +++
src/env.c | 29 ++++++++++++++++++-----------
src/kill.c | 24 ++++++++++++++++--------
src/operand2sig.c | 8 ++++----
src/operand2sig.h | 2 +-
src/timeout.c | 3 +--
tests/misc/kill.sh | 10 +++++++++-
7 files changed, 52 insertions(+), 27 deletions(-)
diff --git a/NEWS b/NEWS
index 20cadf183..f21efc7c0 100644
--- a/NEWS
+++ b/NEWS
@@ -92,6 +92,9 @@ GNU coreutils NEWS -*- outline -*-
This was previously 128KiB and increasing to 256KiB was seen to increase
throughput by 10-20% when reading cached files on modern systems.
+ env,kill,timeout now support unnamed signals. kill(1) for example now
+ supports sending such signals, and env(1) will list them appropriately.
+
SELinux operations in file copy operations are now more efficient,
avoiding unneeded MCS/MLS label translation.
diff --git a/src/env.c b/src/env.c
index 73d9847f4..ed6628f8f 100644
--- a/src/env.c
+++ b/src/env.c
@@ -538,7 +538,6 @@ parse_split_string (char const *str, int *orig_optind,
static void
parse_signal_action_params (char const *arg, bool set_default)
{
- char signame[SIG2STR_MAX];
char *opt_sig;
char *optarg_writable;
@@ -548,8 +547,7 @@ parse_signal_action_params (char const *arg, bool set_default)
Some signals cannot be set to ignore or default (e.g., SIGKILL,
SIGSTOP on most OSes, and SIGCONT on AIX.) - so ignore errors. */
for (int i = 1 ; i <= SIGNUM_BOUND; i++)
- if (sig2str (i, signame) == 0)
- signals[i] = set_default ? DEFAULT_NOERR : IGNORE_NOERR;
+ signals[i] = set_default ? DEFAULT_NOERR : IGNORE_NOERR;
return;
}
@@ -558,7 +556,7 @@ parse_signal_action_params (char const *arg, bool set_default)
opt_sig = strtok (optarg_writable, ",");
while (opt_sig)
{
- int signum = operand2sig (opt_sig, signame);
+ int signum = operand2sig (opt_sig);
/* operand2sig accepts signal 0 (EXIT) - but we reject it. */
if (signum == 0)
error (0, 0, _("%s: invalid signal"), quote (opt_sig));
@@ -607,7 +605,8 @@ reset_signal_handlers (void)
if (dev_debug)
{
char signame[SIG2STR_MAX];
- sig2str (i, signame);
+ if (sig2str (i, signame) != 0)
+ snprintf (signame, sizeof signame, "SIG%d", i);
devmsg ("Reset signal %s (%d) to %s%s\n",
signame, i,
set_to_default ? "DEFAULT" : "IGNORE",
@@ -620,7 +619,6 @@ reset_signal_handlers (void)
static void
parse_block_signal_params (char const *arg, bool block)
{
- char signame[SIG2STR_MAX];
char *opt_sig;
char *optarg_writable;
@@ -647,15 +645,22 @@ parse_block_signal_params (char const *arg, bool block)
opt_sig = strtok (optarg_writable, ",");
while (opt_sig)
{
- int signum = operand2sig (opt_sig, signame);
+ int signum = operand2sig (opt_sig);
/* operand2sig accepts signal 0 (EXIT) - but we reject it. */
if (signum == 0)
error (0, 0, _("%s: invalid signal"), quote (opt_sig));
if (signum <= 0)
usage (exit_failure);
- sigaddset (block ? &block_signals : &unblock_signals, signum);
- sigdelset (block ? &unblock_signals : &block_signals, signum);
+ if (sigaddset (block ? &block_signals : &unblock_signals, signum) == -1)
+ {
+ if (block)
+ error (EXIT_CANCELED, errno,
+ _("failed to block signal %d"), signum);
+ /* else diagnosed in parse_signal_action_params(). */
+ }
+ else
+ sigdelset (block ? &unblock_signals : &block_signals, signum);
opt_sig = strtok (nullptr, ",");
}
@@ -695,7 +700,8 @@ set_signal_proc_mask (void)
if (dev_debug && debug_act)
{
char signame[SIG2STR_MAX];
- sig2str (i, signame);
+ if (sig2str (i, signame) != 0)
+ snprintf (signame, sizeof signame, "SIG%d", i);
devmsg ("signal %s (%d) mask set to %s\n",
signame, i, debug_act);
}
@@ -728,7 +734,8 @@ list_signal_handling (void)
if (! *ignored && ! *blocked)
continue;
- sig2str (i, signame);
+ if (sig2str (i, signame) != 0)
+ snprintf (signame, sizeof signame, "SIG%d", i);
fprintf (stderr, "%-10s (%2d): %s%s%s\n", signame, i,
blocked, connect, ignored);
}
diff --git a/src/kill.c b/src/kill.c
index 9c8b6c191..8b9a2650f 100644
--- a/src/kill.c
+++ b/src/kill.c
@@ -131,11 +131,15 @@ list_signals (bool table, char *const *argv)
if (argv)
for (; *argv; argv++)
{
- signum = operand2sig (*argv, signame);
+ signum = operand2sig (*argv);
if (signum < 0)
status = EXIT_FAILURE;
else
- print_table_row (num_width, signum, name_width, signame);
+ {
+ if (sig2str (signum, signame) != 0)
+ snprintf (signame, sizeof signame, "SIG%d", signum);
+ print_table_row (num_width, signum, name_width, signame);
+ }
}
else
for (signum = 1; signum <= SIGNUM_BOUND; signum++)
@@ -147,16 +151,18 @@ list_signals (bool table, char *const *argv)
if (argv)
for (; *argv; argv++)
{
- signum = operand2sig (*argv, signame);
+ signum = operand2sig (*argv);
if (signum < 0)
status = EXIT_FAILURE;
- else
+ else if (ISDIGIT (**argv))
{
- if (ISDIGIT (**argv))
+ if (sig2str (signum, signame) == 0)
puts (signame);
else
printf ("%d\n", signum);
}
+ else
+ printf ("%d\n", signum);
}
else
for (signum = 1; signum <= SIGNUM_BOUND; signum++)
@@ -190,7 +196,10 @@ send_signals (int signum, char *const *argv)
}
else if (kill (pid, signum) != 0)
{
- error (0, errno, "%s", quote (arg));
+ if (errno == EINVAL)
+ error (0, errno, "%d", signum);
+ else
+ error (0, errno, "%s", quote (arg));
status = EXIT_FAILURE;
}
}
@@ -206,7 +215,6 @@ main (int argc, char **argv)
bool list = false;
bool table = false;
int signum = -1;
- char signame[SIG2STR_MAX];
initialize_main (&argc, &argv);
set_program_name (argv[0]);
@@ -251,7 +259,7 @@ main (int argc, char **argv)
error (0, 0, _("%s: multiple signals specified"), quote (optarg));
usage (EXIT_FAILURE);
}
- signum = operand2sig (optarg, signame);
+ signum = operand2sig (optarg);
if (signum < 0)
usage (EXIT_FAILURE);
break;
diff --git a/src/operand2sig.c b/src/operand2sig.c
index 2a2563c62..b46cb1bed 100644
--- a/src/operand2sig.c
+++ b/src/operand2sig.c
@@ -18,8 +18,8 @@
FIXME: Move this to gnulib/str2sig.c */
-/* Convert OPERAND to a signal number with printable representation SIGNAME.
- Return the signal number, or -1 if unsuccessful. */
+/* Convert OPERAND to a signal number. Return the signal number, or -1 if
+ unsuccessful. */
#include <config.h>
#include <stdio.h>
@@ -32,7 +32,7 @@
#include "operand2sig.h"
extern int
-operand2sig (char const *operand, char *signame)
+operand2sig (char const *operand)
{
int signum;
@@ -82,7 +82,7 @@ operand2sig (char const *operand, char *signame)
free (upcased);
}
- if (signum < 0 || sig2str (signum, signame) != 0)
+ if (0 > signum || signum > SIGNUM_BOUND)
{
error (0, 0, _("%s: invalid signal"), quote (operand));
return -1;
diff --git a/src/operand2sig.h b/src/operand2sig.h
index e46689e7b..3bc551051 100644
--- a/src/operand2sig.h
+++ b/src/operand2sig.h
@@ -15,5 +15,5 @@
You should have received a copy of the GNU General Public License
along with this program. If not, see <https://www.gnu.org/licenses/>. */
-extern int operand2sig (char const *operand, char *signame)
+extern int operand2sig (char const *operand)
_GL_ATTRIBUTE_NONNULL ();
diff --git a/src/timeout.c b/src/timeout.c
index 68d872b12..c102aff85 100644
--- a/src/timeout.c
+++ b/src/timeout.c
@@ -467,7 +467,6 @@ int
main (int argc, char **argv)
{
double timeout;
- char signame[SIG2STR_MAX];
int c;
initialize_main (&argc, &argv);
@@ -488,7 +487,7 @@ main (int argc, char **argv)
break;
case 's':
- term_signal = operand2sig (optarg, signame);
+ term_signal = operand2sig (optarg);
if (term_signal == -1)
usage (EXIT_CANCELED);
break;
diff --git a/tests/misc/kill.sh b/tests/misc/kill.sh
index 69679e5a6..82812eada 100755
--- a/tests/misc/kill.sh
+++ b/tests/misc/kill.sh
@@ -17,7 +17,7 @@
# along with this program. If not, see <https://www.gnu.org/licenses/>.
. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
-print_ver_ kill
+print_ver_ kill seq
# params required
returns_ 1 env kill || fail=1
@@ -60,4 +60,12 @@ returns_ 1 env kill -l -1 || fail=1
returns_ 1 env kill -l -1 0 || fail=1
returns_ 1 env kill -l INVALID TERM || fail=1
+# Verify all signal numbers can be listed
+SIG_LAST_STR=$(env kill -l | tail -n1) || framework_failure_
+SIG_LAST_NUM=$(env kill -l -- "$SIG_LAST_STR") || framework_failure_
+SIG_SEQ=$(env seq -- 0 "$SIG_LAST_NUM") || framework_failure_
+test -n "$SIG_SEQ" || framework_failure_
+env kill -l -- $SIG_SEQ || fail=1
+env kill -t -- $SIG_SEQ || fail=1
+
Exit $fail
--
2.43.0
