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

Documentation for 'kill' should call more attention to using names instead of numbers #12114

Closed
p5pRT opened this issue May 18, 2012 · 24 comments

Comments

@p5pRT
Copy link

p5pRT commented May 18, 2012

Migrated from rt.perl.org#112990 (status was 'resolved')

Searchable as RT112990$

@p5pRT
Copy link
Author

p5pRT commented May 18, 2012

From @dmcbride

Created by @dmcbride

I was looking to see if I could figure out what number to pass in
to kill for a particular signal (SIGINT). Reading the perldoc for
kill wasn't informative at a quick glance, though an hour after asking
on #p5p, someone pointed out the line that says this. It is buried
in a paragraph actually talking about negative signals.

I would suggest that "You may also use a signal name in quotes" should
be expanded upon and get in its own paragraph. Preferably with either
a list of signal names or a reference to where to find those signal names.
If that's "kill -l", it should also suggest whether the SIG prefix is
required (it doesn't seem to be - SIGKILL and KILL both work the same).
And the doc should also mention if negative names work as negative numbers
do.

Note also that even without any other change, the docs are still misleading
when you do notice everything​: it's probably not the signal name being
in quotes, but merely that it's a string​: kill KILL => $$ works as well
as kill 'KILL', $$. I'm sure kill $sig, $$ where $sig contains 'KILL' would
work as well.

I can't seem to find this list referenced in another pod, but I could
have overlooked it.

Note that perlipc does show examples of "kill HUP => $$" - which confused
me until mauke pointed out the well-hidden line in perlfunc that said
this would work.

Also, perlfunc says​:

  Unlike in the shell, if SIGNAL is negative, it kills process
  groups instead of processes. That means you usually want to
  use positive not negative signals. You may also use a signal
  name in quotes.

But perlipc says​:

  Sending a signal to a negative process ID means that you send the
  signal to the entire Unix process group.

So, is it the signal being negative or the process ID or both?

Perl Info

Flags:
    category=docs
    severity=low

Site configuration information for perl 5.15.9:

Configured by dmcbride at Tue Mar 13 16:50:41 MDT 2012.

Summary of my perl5 (revision 5 version 15 subversion 8) configuration:
  Commit id: 2630d42bb94232640db98e1de206d235e3f280fa
  Platform:
    osname=linux, osvers=3.2.1-gentoo-r2, archname=x86_64-linux-thread-multi
    uname='linux naboo 3.2.1-gentoo-r2 #1 smp thu jan 26 07:38:45 mst 2012 x86_64 intel(r) core(tm) i7 cpu 930 @ 2.80ghz genuineintel gnulinux '
    config_args=''
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector'
    ccversion='', gccversion='4.5.3', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/../lib64 /usr/lib/../lib64 /lib /usr/lib /lib64 /usr/lib64 /usr/local/lib64
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc -lgdbm_compat
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=/lib/libc-2.13.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.13'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib -fstack-protector'

Locally applied patches:
    


@INC for perl 5.15.9:
    /home/dmcbride/perl/blead/lib/site_perl/5.15.8/x86_64-linux-thread-multi
    /home/dmcbride/perl/blead/lib/site_perl/5.15.8
    /home/dmcbride/perl/blead/lib/5.15.8/x86_64-linux-thread-multi
    /home/dmcbride/perl/blead/lib/5.15.8
    .


Environment for perl 5.15.9:
    HOME=/home/dmcbride
    LANG=en_US.utf8
    LANGUAGE=
    LC_ALL=en_US.utf8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/dmcbride/bin:/usr/lib/distcc/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/i686-pc-linux-gnu/gcc-bin/4.5.3:/usr/x86_64-pc-linux-gnu/gcc-bin/4.5.3:/share/cvs/bin:/usr/local/bin:/usr/games/bin:/share/bin:/share/darin/bin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented May 18, 2012

From @ikegami

On Fri, May 18, 2012 at 6​:19 PM, Darin McBride <perlbug-followup@​perl.org>wrote​:

So, is it the signal being negative or the process ID or both?

Both negative signals and negative pids send to the process group, IIRC.

@p5pRT
Copy link
Author

p5pRT commented May 18, 2012

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented May 18, 2012

From @Leont

On Sat, May 19, 2012 at 12​:26 AM, Eric Brine <ikegami@​adaelis.com> wrote​:

So, is it the signal being negative or the process ID or both?

Both negative signals and negative pids send to the process group, IIRC.

It seems so, though that's making no sense at all to me. The whole
negative signal thingie is just plain weird. It looks like a case of
hysterical raisins.

Leon

@p5pRT
Copy link
Author

p5pRT commented May 22, 2012

From @dmcbride

On Friday May 18 2012 3​:27​:11 PM you wrote​:

On Fri, May 18, 2012 at 6​:19 PM, Darin McBride <perlbug-
followup@​perl.org>wrote​:

So, is it the signal being negative or the process ID or both?

Both negative signals and negative pids send to the process group, IIRC.

Ok, so what happens with​:

# A
kill INT => 10, -11, 12;

# B
kill -INT => 10, -11, 12;

# C
kill 2 => 10, -11, 12;

# D
kill -2 => 10, -11, 12;

Assuming, of course, proper permissions, these processes exist, they have
process groups beneath them, etc.

I'm assuming A == C and B == D (assuming INT is signal "2" as it is on my
linux box, though I'm unsure if that's guaranteed). I'm not sure that's the
case.

I don't mind attempting a doc patch, but I don't actually understand what it's
doing, so maybe someone who does know can explain it roughly first? I know
"TIAS" - but the whole "process group" thing is something I have less
experience with and wouldn't necessarily know that I've set up properly to
test with.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2012

From @cpansprout

On Tue May 22 12​:21​:04 2012, dmcbride@​cpan.org wrote​:

On Friday May 18 2012 3​:27​:11 PM you wrote​:

On Fri, May 18, 2012 at 6​:19 PM, Darin McBride <perlbug-
followup@​perl.org>wrote​:

So, is it the signal being negative or the process ID or both?

Both negative signals and negative pids send to the process group,
IIRC.

Ok, so what happens with​:

# A
kill INT => 10, -11, 12;

# B
kill -INT => 10, -11, 12;

# C
kill 2 => 10, -11, 12;

# D
kill -2 => 10, -11, 12;

Assuming, of course, proper permissions, these processes exist, they
have
process groups beneath them, etc.

I'm assuming A == C and B == D (assuming INT is signal "2" as it is on
my
linux box, though I'm unsure if that's guaranteed). I'm not sure
that's the
case.

I don't mind attempting a doc patch, but I don't actually understand
what it's
doing, so maybe someone who does know can explain it roughly first? I
know
"TIAS" - but the whole "process group" thing is something I have less
experience with and wouldn't necessarily know that I've set up
properly to
test with.

I haven‘t looked at the code, but I can tell you where it is, if that’s
any help. It’s the ‘case OP_KILL’ in doio.c​:Perl_apply.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented May 22, 2012

From @dmcbride

On Tuesday May 22 2012 1​:43​:01 PM you wrote​:

I haven‘t looked at the code, but I can tell you where it is, if that’s
any help. It’s the ‘case OP_KILL’ in doio.c​:Perl_apply.

Yes, that helps. I'm definitely no expert in perl guts (hey, that'd make a
good name for a perldoc about this! Oh hold on...), but it seems to me from a
cursory read that​:

# A
kill INT => 10, -11, 12;

This will send SIGINT to PIDs 10 and 12, and rely on kill(2) to kill the
process group for 11.

# B
kill -INT => 10, -11, 12;

This will croak C<Unrecognised signal name "-INT">.

*EDIT*​: TIAS says this is wrong. I now suspect it's actually trying to do a
C<kill 0 => 10, -11, 12>. And warnings seem to indicate it​:

$ ~/perl/blead/bin/perl -Mwarnings -lE 'kill -INT => $$'
Argument "-INT" isn't numeric in kill at -e line 1.

Note to self​: "-" isn't ALPHA. Duh. :-)

# C
kill 2 => 10, -11, 12;

This will send signal 2 (presumably SIGINT) just the same as "A" with the same
code path, minus the signal-name-lookup bit.

# D
kill -2 => 10, -11, 12;

This will send signal 2 (presumably SIGINT) to process groups 10, -11, and 12
using killpg(2) if we have it ("BSD") and kill(2) otherwise ("SYSV"). If we
are using killpg, I'm assuming that -11 will fail, but if we're using kill,
-11 will actually send SIGINT to PID 11 (not the group) because we negate it
again.

The fact that we use killpg(2) in one "kill the process group" case, but
kill(2) in the other seems non-intuitive to me.

I'm thinking that

a) C<-INT> and C<-2> should be treated the same. (This should not break
existing code)
b) if the signal is negative, a negative process group should croak, whether
we use killpg(2) or kill(2) under the covers. (This may break existing code
on SYSV platforms if they're relying on the double negative.)
c) if the signal is not negative, a negative process group should use killpg
just the same as if the signal was negative with a positive process group.
(This should not break existing code.) Either that or we always just use
kill(2).

I also have failed to find any tests in the t/ directory that look to be
testing negative signals *or* negative pids. Lots of positive tests, though
:)

While I might take on (a) and (c) above, my confidence level is greatly
diminished by the lack of existing tests combined with my lack of experience
in creating, managing or killing process groups, and thus my lack of
perspective when writing such tests. As for (b), assuming a consensus, I
don't know if a deprecation cycle (and thus a warning) is needed, and, if so,
how to do it. Of course, there may not be consensus, so I leave that to p5p.

@p5pRT
Copy link
Author

p5pRT commented May 22, 2012

From @Leont

On Wed, May 23, 2012 at 12​:08 AM, Darin McBride <dmcbride@​cpan.org> wrote​:

The fact that we use killpg(2) in one "kill the process group" case, but
kill(2) in the other seems non-intuitive to me.

Hysterical raisins​: this codepath does back to perl 2.001 (ffd30a0).
I think we don''t need killpg at all nowadays on anything that's
remotely POSIXy

a) C<-INT> and C<-2> should be treated the same.  (This should not break
existing code)
b) if the signal is negative, a negative process group should croak, whether
we use killpg(2) or kill(2) under the covers.  (This may break existing code
on SYSV platforms if they're relying on the double negative.)

Personally, I'd rather be rid of this whole behavior​: it's unexpected
and unnecessary, but it's probably too late for that.

c) if the signal is not negative, a negative process group should use killpg
just the same as if the signal was negative with a positive process group.
(This should not break existing code.)  Either that or we always just use
kill(2).

The latter makes more sense wrt how everything else works, IMO. POSIX
defines killpg in terms of kill anyway.

Leon

@p5pRT
Copy link
Author

p5pRT commented May 23, 2012

From @dmcbride

Because sometimes a patch spurs more conversation than, well, more
conversation... this is on 5.17.0 blead.

This seems to resolve (a)​: "kill -INT=>$$" gives a funny return code
now, no complaints about numbers even under warnings. It takes Leon's
suggestion for (c) (no more killpg(2)). It explicitly marks (b) as
"undefined" in the docs without (I believe) changing behaviour. It also
completely and utterly fails to address (d)​: adding appropriate tests.

make test_harness still passes, but, again, I haven't seen any tests
regarding this, so my confidence isn't high. I don't seem to have broken
the positives, it's the negatives that need addressing.

@p5pRT
Copy link
Author

p5pRT commented May 23, 2012

From @dmcbride

kill-patch-attempt0.patch
diff --git a/doio.c b/doio.c
index c2b1102..ccb691f 100644
--- a/doio.c
+++ b/doio.c
@@ -1572,6 +1572,7 @@ Perl_apply(pTHX_ I32 type, register SV **mark, register SV **sp)
     const char *s;
     STRLEN len;
     SV ** const oldmark = mark;
+    bool killgp = false;
 
     PERL_ARGS_ASSERT_APPLY;
 
@@ -1682,6 +1683,12 @@ nothing in the core.
 	if (mark == sp)
 	    break;
 	s = SvPVx_const(*++mark, len);
+	if (isALPHA(s[1]) && *s == '-')
+	{
+	    s++;
+	    len--;
+            killgp = true;
+	}
 	if (isALPHA(*s)) {
 	    if (*s == 'S' && s[1] == 'I' && s[2] == 'G') {
 		s += 3;
@@ -1691,12 +1698,18 @@ nothing in the core.
                Perl_croak(aTHX_ "Unrecognized signal name \"%"SVf"\"", SVfARG(*mark));
 	}
 	else
+	{
 	    val = SvIV(*mark);
+	    if (val < 0)
+	    {
+		killgp = true;
+                val = -val;
+	    }
+	}
 	APPLY_TAINT_PROPER();
 	tot = sp - mark;
 #ifdef VMS
 	/* kill() doesn't do process groups (job trees?) under VMS */
-	if (val < 0) val = -val;
 	if (val == SIGKILL) {
 #	    include <starlet.h>
 	    /* Use native sys$delprc() to insure that target process is
@@ -1730,34 +1743,19 @@ nothing in the core.
 	    break;
 	}
 #endif
-	if (val < 0) {
-	    val = -val;
-	    while (++mark <= sp) {
-		I32 proc;
-		SvGETMAGIC(*mark);
-		if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
-		    Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
-		proc = SvIV_nomg(*mark);
-		APPLY_TAINT_PROPER();
-#ifdef HAS_KILLPG
-		if (PerlProc_killpg(proc,val))	/* BSD */
-#else
-		if (PerlProc_kill(-proc,val))	/* SYSV */
-#endif
-		    tot--;
-	    }
-	}
-	else {
-	    while (++mark <= sp) {
-		I32 proc;
-		SvGETMAGIC(*mark);
-		if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
-		    Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
-		proc = SvIV_nomg(*mark);
-		APPLY_TAINT_PROPER();
-		if (PerlProc_kill(proc, val))
-		    tot--;
+	while (++mark <= sp) {
+	    I32 proc;
+	    SvGETMAGIC(*mark);
+	    if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
+		Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
+	    proc = SvIV_nomg(*mark);
+	    if (killgp)
+	    {
+                proc = -proc;
 	    }
+	    APPLY_TAINT_PROPER();
+	    if (PerlProc_kill(proc, val))
+		tot--;
 	}
 	PERL_ASYNC_CHECK();
 	break;
diff --git a/pod/perlfunc.pod b/pod/perlfunc.pod
index 46df6c6..4332ed4 100644
--- a/pod/perlfunc.pod
+++ b/pod/perlfunc.pod
@@ -3171,11 +3171,20 @@ L<perlport> for notes on the portability of this construct.
 Unlike in the shell, if SIGNAL is negative, it kills process groups instead
 of processes.  That means you usually
 want to use positive not negative signals.
-You may also use a signal name in quotes.
+
+You may also use a signal name in quotes.  A negative signal name is the
+same as a negative signal number, killing process groups instead of processes.
+For example, C<kill -KILL, $pgrp> will send C<SIGKILL> to the entire process
+group specified.
 
 The behavior of kill when a I<PROCESS> number is zero or negative depends on
 the operating system.  For example, on POSIX-conforming systems, zero will
-signal the current process group and -1 will signal all processes.
+signal the current process group and -1 will signal all processes, and any
+other negative PROCESS number will act as a negative signal number and
+kill the entire process group specified.
+
+If both the SIGNAL and the PROCESS are negative, the results are undefined.
+A warning may be produced in a future version.
 
 See L<perlipc/"Signals"> for more details.
 

@p5pRT
Copy link
Author

p5pRT commented May 23, 2012

From [Unknown Contact. See original ticket]

Because sometimes a patch spurs more conversation than, well, more
conversation... this is on 5.17.0 blead.

This seems to resolve (a)​: "kill -INT=>$$" gives a funny return code
now, no complaints about numbers even under warnings. It takes Leon's
suggestion for (c) (no more killpg(2)). It explicitly marks (b) as
"undefined" in the docs without (I believe) changing behaviour. It also
completely and utterly fails to address (d)​: adding appropriate tests.

make test_harness still passes, but, again, I haven't seen any tests
regarding this, so my confidence isn't high. I don't seem to have broken
the positives, it's the negatives that need addressing.

@p5pRT
Copy link
Author

p5pRT commented May 26, 2012

From @cpansprout

On Tue May 22 21​:42​:23 2012, dmcbride@​cpan.org wrote​:

Because sometimes a patch spurs more conversation than, well, more
conversation... this is on 5.17.0 blead.

This seems to resolve (a)​: "kill -INT=>$$" gives a funny return code
now, no complaints about numbers even under warnings. It takes Leon's
suggestion for (c) (no more killpg(2)). It explicitly marks (b) as
"undefined" in the docs without (I believe) changing behaviour.

I think all that is a good idea.

It also
completely and utterly fails to address (d)​: adding appropriate tests.

But for your patch to be applied, we need tests at least for the
behaviour it is changing. Tests for other untested aspects of kill are
nice, but not necessary to get this patch in.

I do have a couple of observations, though​:

@​@​ -1682,6 +1683,12 @​@​ nothing in the core.
  if (mark == sp)
  break;
  s = SvPVx_const(*++mark, len);
+ if (isALPHA(s[1]) && *s == '-')
+ {
+ s++;
+ len--;
+ killgp = true;
+ }
  if (isALPHA(*s)) {
  if (*s == 'S' && s[1] == 'I' && s[2] == 'G') {
  s += 3;

Why not put the if(*s == '-') inside the existing if(isALPHA) alpha, as
a nested if()? That way isALPHA is only checked once.

the operating system. For example, on POSIX-conforming systems, zero will
-signal the current process group and -1 will signal all processes.
+signal the current process group and -1 will signal all processes, and any
+other negative PROCESS number will act as a negative signal number and
+kill the entire process group specified.

I would suggest s/group and -1/group, -1/.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented May 27, 2012

From @dmcbride

On Saturday May 26 2012 11​:16​:26 AM you wrote​:

On Tue May 22 21​:42​:23 2012, dmcbride@​cpan.org wrote​:

Because sometimes a patch spurs more conversation than, well, more
conversation... this is on 5.17.0 blead.

This seems to resolve (a)​: "kill -INT=>$$" gives a funny return code
now, no complaints about numbers even under warnings. It takes Leon's
suggestion for (c) (no more killpg(2)). It explicitly marks (b) as
"undefined" in the docs without (I believe) changing behaviour.

I think all that is a good idea.

It also
completely and utterly fails to address (d)​: adding appropriate tests.

But for your patch to be applied, we need tests at least for the
behaviour it is changing. Tests for other untested aspects of kill are
nice, but not necessary to get this patch in.

Is there someone on the list that has more understanding of process groups
than I who could help? (That is, *any* understanding would suffice.)

I agree - I was hoping this patch would encourage that, I knew some tests
would be required somewhere for it :-)

I do have a couple of observations, though​:

@​@​ -1682,6 +1683,12 @​@​ nothing in the core.
if (mark == sp)
break;
s = SvPVx_const(*++mark, len);
+ if (isALPHA(s[1]) && *s == '-')
+ {
+ s++;
+ len--;
+ killgp = true;
+ }
if (isALPHA(*s)) {
if (*s == 'S' && s[1] == 'I' && s[2] == 'G') {
s += 3;

Why not put the if(*s == '-') inside the existing if(isALPHA) alpha, as
a nested if()? That way isALPHA is only checked once.

Except that the "if(isALPHA(*s))" you're referring to is checking s[0], not
s[1]. It's true that if s[0] is '-' and s[1] isALPHA, we're going to check
s[1] twice (once as s[1], once as *s), though I'm not entirely sure how to
alleviate that without changing behaviour. And I'm not sure that it matters
that much that kill is taking an extra few dozen CPU cycles - I'm not sure
that kill is something that is generally run in a tight loop.

If our isALPHA were to check s[1] first, passing in "2INT" would produce
"Unrecognized signal name "2INT" instead of killing with signal 2, as it does
today (at least without fatal warnings)​:

if (isALPHA(s[1])) {
  if (*s == '-') {
  s++;
  len--;
  killgp = true;
  }
  if (*s == 'S' ...

Outside of the if's, we care that it's -[a-zA-Z] so that we don't eliminate
the - for numerics, so we still have to check s[1].

We can probably switch the order - check *s before s[1] - as that will likely
be that tiny bit faster (checking against a single constant rather than a
table lookup). But it still seems to me to be two separate checks that can't
be merged.

So, unless I'm missing something, I'm not sure how to nest these if's.

the operating system. For example, on POSIX-conforming systems, zero will
-signal the current process group and -1 will signal all processes.
+signal the current process group and -1 will signal all processes, and any
+other negative PROCESS number will act as a negative signal number and
+kill the entire process group specified.

I would suggest s/group and -1/group, -1/.

Good catch.

@p5pRT
Copy link
Author

p5pRT commented May 27, 2012

From @cpansprout

On Sat May 26 22​:44​:13 2012, dmcbride@​cpan.org wrote​:

On Saturday May 26 2012 11​:16​:26 AM you wrote​:

On Tue May 22 21​:42​:23 2012, dmcbride@​cpan.org wrote​:

Because sometimes a patch spurs more conversation than, well, more
conversation... this is on 5.17.0 blead.

This seems to resolve (a)​: "kill -INT=>$$" gives a funny return
code
now, no complaints about numbers even under warnings. It takes
Leon's
suggestion for (c) (no more killpg(2)). It explicitly marks (b)
as
"undefined" in the docs without (I believe) changing behaviour.

I think all that is a good idea.

It also
completely and utterly fails to address (d)​: adding appropriate
tests.

But for your patch to be applied, we need tests at least for the
behaviour it is changing. Tests for other untested aspects of kill
are
nice, but not necessary to get this patch in.

Is there someone on the list that has more understanding of process
groups
than I who could help? (That is, *any* understanding would suffice.)

All I know is based on kill’s documentation, I’m afraid.

If it’s not testable, then two pairs of eyeballs are probably
sufficient. I’ll read through it one more time, and then wait a bit for
someone else to speak up.

I have noticed that a *lot* of IPC stuff is simply not tested, because
it can’t be (or testing it is not at all trivial).

I agree - I was hoping this patch would encourage that, I knew some
tests
would be required somewhere for it :-)

I do have a couple of observations, though​:

@​@​ -1682,6 +1683,12 @​@​ nothing in the core.
if (mark == sp)
break;
s = SvPVx_const(*++mark, len);
+ if (isALPHA(s[1]) && *s == '-')
+ {
+ s++;
+ len--;
+ killgp = true;
+ }
if (isALPHA(*s)) {
if (*s == 'S' && s[1] == 'I' && s[2] == 'G') {
s += 3;

Why not put the if(*s == '-') inside the existing if(isALPHA) alpha,
as
a nested if()? That way isALPHA is only checked once.

Except that the "if(isALPHA(*s))" you're referring to is checking
s[0], not
s[1].

OK, never mind then. I misread the code.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented May 27, 2012

From @hvds

Darin McBride <dmcbride@​cpan.org> wrote​:
:On Saturday May 26 2012 11​:16​:26 AM [Father Chrysostomos] wrote​:
:> On Tue May 22 21​:42​:23 2012, dmcbride@​cpan.org wrote​:
:> > Because sometimes a patch spurs more conversation than, well, more
:> > conversation... this is on 5.17.0 blead.
:> >
:> > This seems to resolve (a)​: "kill -INT=>$$" gives a funny return code
:> > now, no complaints about numbers even under warnings. It takes Leon's
:> > suggestion for (c) (no more killpg(2)). It explicitly marks (b) as
:> > "undefined" in the docs without (I believe) changing behaviour.
:>
:> I think all that is a good idea.
:>
:> > It also
:> > completely and utterly fails to address (d)​: adding appropriate tests.
:>
:> But for your patch to be applied, we need tests at least for the
:> behaviour it is changing. Tests for other untested aspects of kill are
:> nice, but not necessary to get this patch in.
:
:Is there someone on the list that has more understanding of process groups
:than I who could help? (That is, *any* understanding would suffice.)
:
:I agree - I was hoping this patch would encourage that, I knew some tests
:would be required somewhere for it :-)
:
:> I do have a couple of observations, though​:
:>
:> @​@​ -1682,6 +1683,12 @​@​ nothing in the core.
:> if (mark == sp)
:> break;
:> s = SvPVx_const(*++mark, len);
:> + if (isALPHA(s[1]) && *s == '-')
:> + {
:> + s++;
:> + len--;
:> + killgp = true;
:> + }
:> if (isALPHA(*s)) {
:> if (*s == 'S' && s[1] == 'I' && s[2] == 'G') {
:> s += 3;
:>
:> Why not put the if(*s == '-') inside the existing if(isALPHA) alpha, as
:> a nested if()? That way isALPHA is only checked once.
:
:Except that the "if(isALPHA(*s))" you're referring to is checking s[0], not
:s[1]. It's true that if s[0] is '-' and s[1] isALPHA, we're going to check
:s[1] twice (once as s[1], once as *s), though I'm not entirely sure how to
:alleviate that without changing behaviour. And I'm not sure that it matters
:that much that kill is taking an extra few dozen CPU cycles - I'm not sure
:that kill is something that is generally run in a tight loop.
:
:If our isALPHA were to check s[1] first, passing in "2INT" would produce
:"Unrecognized signal name "2INT" instead of killing with signal 2, as it does
:today (at least without fatal warnings)​:
:
:if (isALPHA(s[1])) {
: if (*s == '-') {
: s++;
: len--;
: killgp = true;
: }
: if (*s == 'S' ...
:
:Outside of the if's, we care that it's -[a-zA-Z] so that we don't eliminate
:the - for numerics, so we still have to check s[1].
:
:We can probably switch the order - check *s before s[1] - as that will likely
:be that tiny bit faster (checking against a single constant rather than a
:table lookup). But it still seems to me to be two separate checks that can't
:be merged.

Putting the micro-optimization to one side, I think this use of s[1] may
read past end of string​: it should either check len, or know that s[0] is
non-zero before reading it.

(If there is some reason this is known to be safe due to prior checks,
then it would be helpful to put a comment in the code explaining that.)

Hugo

@p5pRT
Copy link
Author

p5pRT commented May 28, 2012

From @hvds

Darin McBride <dmcbride@​cpan.org> wrote​:
:On Sunday May 27 2012 9​:13​:28 AM hv@​crypt.org wrote​:
:> Putting the micro-optimization to one side, I think this use of s[1] may
:> read past end of string​: it should either check len, or know that s[0] is
:> non-zero before reading it.
:
:It's also following the logic below it that also does not check explicitly.
:However, with the micro-optimisation, if s[0] is nul, it will fail the == '-'
:check, and not check s[1]. Much like the check that the first three characters
:are SIG.
:
:So, the only real assumptions here are that s[0] is valid and s contains a
:nul-terminated string. Which, as I said, is the same assumption in the
:if(isALPHA(*s)) line that was already there. I don't know that SvPVx_const
:always returns a valid nul-terminated string, I'm assuming it based on the
:assumptions already there.
:
:(* I know that "nul-terminated" and "no nuls except at the end of the string"
:are not the same thing, but for the purposes of this patch, they're close
:enough.)

I'm not sure I understand what you're saying here, and suspect I may not
have made myself clear; for the previously quoted fragment I think all
that's needed is to reverse the order of checks​:

- if (isALPHA(s[1]) && *s == '-')
+ if (*s == '-' && isALPHA(s[1]))

.. so you don't go trying to read s[1] until you're sure s[0] != 0.

I don't remember offhand the circumstances in which it is possible to get
a string that isn't nul-terminated. As far as I know for this case it'd
only happen if someone got loose with their XS code, in which case we can
probably just tell them not to do that.

:> (If there is some reason this is known to be safe due to prior checks,
:> then it would be helpful to put a comment in the code explaining that.)
:
:Should I also add the same comment to the code below this point?

I'm not sure what code we're talking about now, but if we're doing something
that looks unsafe for a good reason, a comment explaining is helpful. If
we're doing it for no good reason, probably better to amend it.

Hugo

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2012

From @cpansprout

On Sat May 26 22​:49​:30 2012, sprout wrote​:

On Sat May 26 22​:44​:13 2012, dmcbride@​cpan.org wrote​:

On Saturday May 26 2012 11​:16​:26 AM you wrote​:

But for your patch to be applied, we need tests at least for the
behaviour it is changing. Tests for other untested aspects of kill
are
nice, but not necessary to get this patch in.

Is there someone on the list that has more understanding of process
groups
than I who could help? (That is, *any* understanding would suffice.)

All I know is based on kill’s documentation, I’m afraid.

If it’s not testable, then two pairs of eyeballs are probably
sufficient. I’ll read through it one more time, and then wait a bit for
someone else to speak up.

Attached is a slightly tweaked version, based on Hugo’s input and my doc
suggestion.

If you can write a commit message for it, I’ll apply it. (I know, I
could copy and paste parts of the thread, but I’m on my way to bed now.)

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2012

From @cpansprout

Inline Patch
diff --git a/doio.c b/doio.c
index c2b1102..ccb691f 100644
--- a/doio.c
+++ b/doio.c
@@ -1572,6 +1572,7 @@ Perl_apply(pTHX_ I32 type, register SV **mark, register SV **sp)
     const char *s;
     STRLEN len;
     SV ** const oldmark = mark;
+    bool killgp = false;
 
     PERL_ARGS_ASSERT_APPLY;
 
@@ -1682,6 +1683,12 @@ nothing in the core.
 	if (mark == sp)
 	    break;
 	s = SvPVx_const(*++mark, len);
+	if (*s == '-' && isALPHA(s[1]))
+	{
+	    s++;
+	    len--;
+            killgp = true;
+	}
 	if (isALPHA(*s)) {
 	    if (*s == 'S' && s[1] == 'I' && s[2] == 'G') {
 		s += 3;
@@ -1691,12 +1698,18 @@ nothing in the core.
                Perl_croak(aTHX_ "Unrecognized signal name \"%"SVf"\"", SVfARG(*mark));
 	}
 	else
+	{
 	    val = SvIV(*mark);
+	    if (val < 0)
+	    {
+		killgp = true;
+                val = -val;
+	    }
+	}
 	APPLY_TAINT_PROPER();
 	tot = sp - mark;
 #ifdef VMS
 	/* kill() doesn't do process groups (job trees?) under VMS */
-	if (val < 0) val = -val;
 	if (val == SIGKILL) {
 #	    include <starlet.h>
 	    /* Use native sys$delprc() to insure that target process is
@@ -1730,34 +1743,19 @@ nothing in the core.
 	    break;
 	}
 #endif
-	if (val < 0) {
-	    val = -val;
-	    while (++mark <= sp) {
-		I32 proc;
-		SvGETMAGIC(*mark);
-		if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
-		    Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
-		proc = SvIV_nomg(*mark);
-		APPLY_TAINT_PROPER();
-#ifdef HAS_KILLPG
-		if (PerlProc_killpg(proc,val))	/* BSD */
-#else
-		if (PerlProc_kill(-proc,val))	/* SYSV */
-#endif
-		    tot--;
-	    }
-	}
-	else {
-	    while (++mark <= sp) {
-		I32 proc;
-		SvGETMAGIC(*mark);
-		if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
-		    Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
-		proc = SvIV_nomg(*mark);
-		APPLY_TAINT_PROPER();
-		if (PerlProc_kill(proc, val))
-		    tot--;
+	while (++mark <= sp) {
+	    I32 proc;
+	    SvGETMAGIC(*mark);
+	    if (!(SvIOK(*mark) || SvNOK(*mark) || looks_like_number(*mark)))
+		Perl_croak(aTHX_ "Can't kill a non-numeric process ID");
+	    proc = SvIV_nomg(*mark);
+	    if (killgp)
+	    {
+                proc = -proc;
 	    }
+	    APPLY_TAINT_PROPER();
+	    if (PerlProc_kill(proc, val))
+		tot--;
 	}
 	PERL_ASYNC_CHECK();
 	break;
diff --git a/pod/perlfunc.pod b/pod/perlfunc.pod
index 46df6c6..4332ed4 100644
--- a/pod/perlfunc.pod
+++ b/pod/perlfunc.pod
@@ -3171,11 +3171,20 @@ L<perlport> for notes on the portability of this construct.
 Unlike in the shell, if SIGNAL is negative, it kills process groups instead
 of processes.  That means you usually
 want to use positive not negative signals.
-You may also use a signal name in quotes.
+
+You may also use a signal name in quotes.  A negative signal name is the
+same as a negative signal number, killing process groups instead of processes.
+For example, C<kill -KILL, $pgrp> will send C<SIGKILL> to the entire process
+group specified.
 
 The behavior of kill when a I<PROCESS> number is zero or negative depends on
 the operating system.  For example, on POSIX-conforming systems, zero will
-signal the current process group and -1 will signal all processes.
+signal the current process group, -1 will signal all processes, and any
+other negative PROCESS number will act as a negative signal number and
+kill the entire process group specified.
+
+If both the SIGNAL and the PROCESS are negative, the results are undefined.
+A warning may be produced in a future version.
 
 See L<perlipc/"Signals"> for more details.
 

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2012

From @dmcbride

On Friday June 22 2012 11​:50​:03 PM you wrote​:

Attached is a slightly tweaked version, based on Hugo’s input and my doc
suggestion.

This tweaked version looks exactly like what I have in git right now (of
course, without a commit bit, it's only local). So I must have already taken
that input, and then managed to drop it. Sorry, my bad.

If you can write a commit message for it, I’ll apply it. (I know, I
could copy and paste parts of the thread, but I’m on my way to bed now.)

What I used here was​:

  Simplify kill implementation which allows the kill documentation to
  be both simplified and clarified.

I'm not sure if that's generally sufficient, but it may be close.

@p5pRT
Copy link
Author

p5pRT commented Jun 23, 2012

From @cpansprout

On Sat Jun 23 06​:13​:57 2012, dmcbride@​cpan.org wrote​:

What I used here was​:

Simplify kill implementation which allows the kill documentation

to
be both simplified and clarified.

I'm not sure if that's generally sufficient, but it may be close.

If you could describe exactly what behaviour changed, that would be better.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jun 25, 2012

From @dmcbride

On Saturday June 23 2012 9​:30​:32 AM you wrote​:

On Sat Jun 23 06​:13​:57 2012, dmcbride@​cpan.org wrote​:

What I used here was​:
Simplify kill implementation which allows the kill documentation

to

be both simplified and clarified\.

I'm not sure if that's generally sufficient, but it may be close.

If you could describe exactly what behaviour changed, that would be better.

Ok. Looking through git log, the detail is all over the map, so I wasn't yet
sure what defines bigger vs smaller log requirements. I'm sure I'll figure this
out over time. :-)

Clean up kill implementation and clear up the docs in perlfunc to be less
ambiguous and encompass more of its behaviour.

a) kill -INT => $pid;

was surprisingly doing a "kill 0, $pid" instead of being the same as "kill -2,
$pid" (killing the process group for $pid with an interrupt). Now negative
signal names will be allowed and be the same as if the name was replaced with
the signal number it represents.

b) remove all calls to killpg() as killpg is defined in terms of kill anyway.

c) Clarify the use of signal names vs numbers in perlfunc so that using names
is not so well hidden, as well as explaining the usage of negative signal
numbers as well as negative process IDs.

@p5pRT
Copy link
Author

p5pRT commented Jun 26, 2012

From @cpansprout

On Mon Jun 25 10​:56​:56 2012, dmcbride@​cpan.org wrote​:

On Saturday June 23 2012 9​:30​:32 AM you wrote​:

On Sat Jun 23 06​:13​:57 2012, dmcbride@​cpan.org wrote​:

What I used here was​:
Simplify kill implementation which allows the kill
documentation

to

be both simplified and clarified\.

I'm not sure if that's generally sufficient, but it may be close.

If you could describe exactly what behaviour changed, that would be
better.

Ok. Looking through git log, the detail is all over the map, so I
wasn't yet
sure what defines bigger vs smaller log requirements. I'm sure I'll
figure this
out over time. :-)

Clean up kill implementation and clear up the docs in perlfunc to be
less
ambiguous and encompass more of its behaviour.

a) kill -INT => $pid;

was surprisingly doing a "kill 0, $pid" instead of being the same as
"kill -2,
$pid" (killing the process group for $pid with an interrupt). Now
negative
signal names will be allowed and be the same as if the name was
replaced with
the signal number it represents.

b) remove all calls to killpg() as killpg is defined in terms of kill
anyway.

c) Clarify the use of signal names vs numbers in perlfunc so that
using names
is not so well hidden, as well as explaining the usage of negative
signal
numbers as well as negative process IDs.

Thank you. Applied as c2fd40c.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jun 26, 2012

@cpansprout - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Jun 26, 2012
@p5pRT
Copy link
Author

p5pRT commented Feb 11, 2014

From @bulk88

On Tue Jun 26 09​:58​:18 2012, sprout wrote​:

Thank you. Applied as c2fd40c.

This commit broke Win32 Perl, ticket filed at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121230 .

--
bulk88 ~ bulk88 at hotmail.com

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

No branches or pull requests

1 participant