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
Comments
From @dmcbrideCreated by @dmcbrideI was looking to see if I could figure out what number to pass in I would suggest that "You may also use a signal name in quotes" should Note also that even without any other change, the docs are still misleading I can't seem to find this list referenced in another pod, but I could Note that perlipc does show examples of "kill HUP => $$" - which confused Also, perlfunc says: Unlike in the shell, if SIGNAL is negative, it kills process But perlipc says: Sending a signal to a negative process ID means that you send the So, is it the signal being negative or the process ID or both? Perl Info
|
From @ikegamiOn Fri, May 18, 2012 at 6:19 PM, Darin McBride <perlbug-followup@perl.org>wrote:
Both negative signals and negative pids send to the process group, IIRC. |
The RT System itself - Status changed from 'new' to 'open' |
From @LeontOn Sat, May 19, 2012 at 12:26 AM, Eric Brine <ikegami@adaelis.com> wrote:
It seems so, though that's making no sense at all to me. The whole Leon |
From @dmcbrideOn Friday May 18 2012 3:27:11 PM you wrote:
Ok, so what happens with: # A # B # C # D Assuming, of course, proper permissions, these processes exist, they have I'm assuming A == C and B == D (assuming INT is signal "2" as it is on my I don't mind attempting a doc patch, but I don't actually understand what it's |
From @cpansproutOn Tue May 22 12:21:04 2012, dmcbride@cpan.org wrote:
I haven‘t looked at the code, but I can tell you where it is, if that’s -- Father Chrysostomos |
From @dmcbrideOn Tuesday May 22 2012 1:43:01 PM you wrote:
Yes, that helps. I'm definitely no expert in perl guts (hey, that'd make a
This will send SIGINT to PIDs 10 and 12, and rely on kill(2) to kill the
This will croak C<Unrecognised signal name "-INT">. *EDIT*: TIAS says this is wrong. I now suspect it's actually trying to do a $ ~/perl/blead/bin/perl -Mwarnings -lE 'kill -INT => $$' Note to self: "-" isn't ALPHA. Duh. :-)
This will send signal 2 (presumably SIGINT) just the same as "A" with the same
This will send signal 2 (presumably SIGINT) to process groups 10, -11, and 12 The fact that we use killpg(2) in one "kill the process group" case, but I'm thinking that a) C<-INT> and C<-2> should be treated the same. (This should not break I also have failed to find any tests in the t/ directory that look to be While I might take on (a) and (c) above, my confidence level is greatly |
From @LeontOn Wed, May 23, 2012 at 12:08 AM, Darin McBride <dmcbride@cpan.org> wrote:
Hysterical raisins: this codepath does back to perl 2.001 (ffd30a0).
Personally, I'd rather be rid of this whole behavior: it's unexpected
The latter makes more sense wrt how everything else works, IMO. POSIX Leon |
From @dmcbrideBecause sometimes a patch spurs more conversation than, well, more This seems to resolve (a): "kill -INT=>$$" gives a funny return code make test_harness still passes, but, again, I haven't seen any tests |
From @dmcbridekill-patch-attempt0.patchdiff --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.
|
From [Unknown Contact. See original ticket]Because sometimes a patch spurs more conversation than, well, more This seems to resolve (a): "kill -INT=>$$" gives a funny return code make test_harness still passes, but, again, I haven't seen any tests |
From @cpansproutOn Tue May 22 21:42:23 2012, dmcbride@cpan.org wrote:
I think all that is a good idea.
But for your patch to be applied, we need tests at least for the I do have a couple of observations, though: @@ -1682,6 +1683,12 @@ nothing in the core. Why not put the if(*s == '-') inside the existing if(isALPHA) alpha, as the operating system. For example, on POSIX-conforming systems, zero will I would suggest s/group and -1/group, -1/. -- Father Chrysostomos |
From @dmcbrideOn Saturday May 26 2012 11:16:26 AM you wrote:
Is there someone on the list that has more understanding of process groups I agree - I was hoping this patch would encourage that, I knew some tests
Except that the "if(isALPHA(*s))" you're referring to is checking s[0], not If our isALPHA were to check s[1] first, passing in "2INT" would produce if (isALPHA(s[1])) { Outside of the if's, we care that it's -[a-zA-Z] so that we don't eliminate We can probably switch the order - check *s before s[1] - as that will likely So, unless I'm missing something, I'm not sure how to nest these if's.
Good catch. |
From @cpansproutOn Sat May 26 22:44:13 2012, dmcbride@cpan.org wrote:
All I know is based on kill’s documentation, I’m afraid. If it’s not testable, then two pairs of eyeballs are probably I have noticed that a *lot* of IPC stuff is simply not tested, because
OK, never mind then. I misread the code. -- Father Chrysostomos |
From @hvdsDarin McBride <dmcbride@cpan.org> wrote: Putting the micro-optimization to one side, I think this use of s[1] may (If there is some reason this is known to be safe due to prior checks, Hugo |
From @hvdsDarin McBride <dmcbride@cpan.org> wrote: I'm not sure I understand what you're saying here, and suspect I may not - if (isALPHA(s[1]) && *s == '-') .. 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 :> (If there is some reason this is known to be safe due to prior checks, I'm not sure what code we're talking about now, but if we're doing something Hugo |
From @cpansproutOn Sat May 26 22:49:30 2012, sprout wrote:
Attached is a slightly tweaked version, based on Hugo’s input and my doc If you can write a commit message for it, I’ll apply it. (I know, I -- Father Chrysostomos |
From @cpansproutInline Patchdiff --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.
|
From @dmcbrideOn Friday June 22 2012 11:50:03 PM you wrote:
This tweaked version looks exactly like what I have in git right now (of
What I used here was: Simplify kill implementation which allows the kill documentation to I'm not sure if that's generally sufficient, but it may be close. |
From @cpansproutOn Sat Jun 23 06:13:57 2012, dmcbride@cpan.org wrote:
If you could describe exactly what behaviour changed, that would be better. -- Father Chrysostomos |
From @dmcbrideOn Saturday June 23 2012 9:30:32 AM you wrote:
Ok. Looking through git log, the detail is all over the map, so I wasn't yet Clean up kill implementation and clear up the docs in perlfunc to be less a) kill -INT => $pid; was surprisingly doing a "kill 0, $pid" instead of being the same as "kill -2, 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 |
From @cpansproutOn Mon Jun 25 10:56:56 2012, dmcbride@cpan.org wrote:
Thank you. Applied as c2fd40c. -- Father Chrysostomos |
@cpansprout - Status changed from 'open' to 'resolved' |
From @bulk88On Tue Jun 26 09:58:18 2012, sprout wrote:
This commit broke Win32 Perl, ticket filed at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=121230 . -- |
Migrated from rt.perl.org#112990 (status was 'resolved')
Searchable as RT112990$
The text was updated successfully, but these errors were encountered: