Skip Menu |

Subject: Documentation for 'kill' should call more attention to using names instead of numbers
Date: Fri, 18 May 2012 16:18:59 -0600 (MDT)
To: perlbug [...] perl.org
From: dmcbride [...] cpan.org
Download (untitled) / with headers
text/plain 4.7k
This is a bug report for perl from dmcbride@cpan.org, generated with the help of perlbug 1.39 running under perl 5.15.9. ----------------------------------------------------------------- [Please describe your issue here] 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? [Please do not change anything below this line] ----------------------------------------------------------------- --- 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
CC: bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Fri, 18 May 2012 18:26:40 -0400
To: perl5-porters [...] perl.org
From: Eric Brine <ikegami [...] adaelis.com>
Download (untitled) / with headers
text/plain 223b
On Fri, May 18, 2012 at 6:19 PM, Darin McBride <perlbug-followup@perl.org> wrote:
Show quoted text
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.

CC: perl5-porters [...] perl.org, bugs-bitbucket [...] rt.perl.org
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Sat, 19 May 2012 00:46:13 +0200
To: Eric Brine <ikegami [...] adaelis.com>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 381b
On Sat, May 19, 2012 at 12:26 AM, Eric Brine <ikegami@adaelis.com> wrote: Show quoted text
>> 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
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Tue, 22 May 2012 13:20:12 -0600
To: perlbug-followup [...] perl.org
From: Darin McBride <dmcbride [...] cpan.org>
Download (untitled) / with headers
text/plain 1003b
On Friday May 18 2012 3:27:11 PM you wrote: Show quoted text
> On Fri, May 18, 2012 at 6:19 PM, Darin McBride <perlbug-
followup@perl.org>wrote: Show quoted text
> > 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.
Download signature.asc
application/pgp-signature 198b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.2k
On Tue May 22 12:21:04 2012, dmcbride@cpan.org wrote: Show quoted text
> 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
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Tue, 22 May 2012 16:08:24 -0600
To: perlbug-followup [...] perl.org
From: Darin McBride <dmcbride [...] cpan.org>
Download (untitled) / with headers
text/plain 2.6k
On Tuesday May 22 2012 1:43:01 PM you wrote: Show quoted text
> 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: Show quoted text
> > # 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. Show quoted text
> > # 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. :-) Show quoted text
> > # 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. Show quoted text
> > # 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.
Download signature.asc
application/pgp-signature 198b

Message body not shown because it is not plain text.

CC: perlbug-followup [...] perl.org
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Wed, 23 May 2012 01:44:44 +0200
To: Darin McBride <dmcbride [...] cpan.org>
From: Leon Timmermans <fawaka [...] gmail.com>
Download (untitled) / with headers
text/plain 1.1k
On Wed, May 23, 2012 at 12:08 AM, Darin McBride <dmcbride@cpan.org> wrote: Show quoted text
> 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 (ffd30a0b). I think we don''t need killpg at all nowadays on anything that's remotely POSIXy Show quoted text
> 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. Show quoted text
> 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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 667b
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.
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.
RT-Send-CC: perl5-porters [...] perl.org, dmcbride [...] cpan.org
Download (untitled) / with headers
text/plain 1.5k
On Tue May 22 21:42:23 2012, dmcbride@cpan.org wrote: Show quoted text
> 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. Show quoted text
> 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
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Sat, 26 May 2012 23:43:21 -0600
To: perlbug-followup [...] perl.org
From: Darin McBride <dmcbride [...] cpan.org>
Download (untitled) / with headers
text/plain 3.1k
On Saturday May 26 2012 11:16:26 AM you wrote: Show quoted text
> 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 :-) Show quoted text
> 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. Show quoted text
> 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.
Download signature.asc
application/pgp-signature 198b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 2.2k
On Sat May 26 22:44:13 2012, dmcbride@cpan.org wrote: Show quoted text
> 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). Show quoted text
> > 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
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Sun, 27 May 2012 09:13:28 +0100
To: Darin McBride <dmcbride [...] cpan.org>
From: hv [...] crypt.org
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
CC: perlbug-followup [...] perl.org
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Mon, 28 May 2012 08:56:08 +0100
To: Darin McBride <dmcbride [...] cpan.org>
From: hv [...] crypt.org
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
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1018b
On Sat May 26 22:49:30 2012, sprout wrote: Show quoted text
> 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
Download tweaked patch.text
text/plain 3.4k
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.
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Sat, 23 Jun 2012 07:13:10 -0600
To: perlbug-followup [...] perl.org
From: Darin McBride <dmcbride [...] cpan.org>
Download (untitled) / with headers
text/plain 699b
On Friday June 22 2012 11:50:03 PM you wrote: Show quoted text
> 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. Show quoted text
> 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.
Download signature.asc
application/pgp-signature 198b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 371b
On Sat Jun 23 06:13:57 2012, dmcbride@cpan.org wrote: Show quoted text
> 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
Subject: Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers
Date: Mon, 25 Jun 2012 11:56:18 -0600
To: perlbug-followup [...] perl.org
From: Darin McBride <dmcbride [...] cpan.org>
Download (untitled) / with headers
text/plain 1.2k
On Saturday June 23 2012 9:30:32 AM you wrote: Show quoted text
> 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.
Download signature.asc
application/pgp-signature 198b

Message body not shown because it is not plain text.

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.4k
On Mon Jun 25 10:56:56 2012, dmcbride@cpan.org wrote: Show quoted text
> 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 c2fd40cb0. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 219b
On Tue Jun 26 09:58:18 2012, sprout wrote: Show quoted text
> > Thank you. Applied as c2fd40cb0. >
This commit broke Win32 Perl, ticket filed at https://rt.perl.org/Ticket/Display.html?id=121230 . -- bulk88 ~ bulk88 at hotmail.com


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org