Mailing List Archive: Perl: porters

[perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers

Index | Next | Previous | View Threaded

perlbug-followup at perl

May 18, 2012, 3:19 PM

Post #1 of 15 (176 views)
Permalink

[perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers

# New Ticket Created by Darin McBride
# Please include the string: [perl #112990]
# in the subject line of all future correspondence about this issue.
# <URL: https://rt.perl.org:443/rt3/Ticket/Display.html?id=112990 >

This is a bug report for perl from dmcbride [at] cpan,
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

ikegami at adaelis

May 18, 2012, 3:26 PM

Post #2 of 15 (174 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Fri, May 18, 2012 at 6:19 PM, Darin McBride <perlbug-followup [at] perl>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.

fawaka at gmail

May 18, 2012, 3:46 PM

Post #3 of 15 (175 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Sat, May 19, 2012 at 12:26 AM, Eric Brine <ikegami [at] adaelis> 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

dmcbride at cpan

May 22, 2012, 12:20 PM

Post #4 of 15 (170 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Friday May 18 2012 3:27:11 PM you wrote:
> On Fri, May 18, 2012 at 6:19 PM, Darin McBride <perlbug-
followup [at] perl>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.

Attachments:

signature.asc (0.19 KB)

perlbug-followup at perl

May 22, 2012, 1:43 PM

Post #5 of 15 (170 views)
Permalink

[perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Tue May 22 12:21:04 2012, dmcbride [at] cpan 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 [at] perl>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

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112990

dmcbride at cpan

May 22, 2012, 3:08 PM

Post #6 of 15 (171 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

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.

Attachments:

signature.asc (0.19 KB)

fawaka at gmail

May 22, 2012, 4:44 PM

Post #7 of 15 (169 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Wed, May 23, 2012 at 12:08 AM, Darin McBride <dmcbride [at] cpan> 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 (ffd30a0b).
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

perlbug-comment at perl

May 22, 2012, 9:42 PM

Post #8 of 15 (171 views)
Permalink

[perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

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.

Attachments:

kill-patch-attempt0.patch (3.49 KB)

perlbug-followup at perl

May 26, 2012, 11:16 AM

Post #9 of 15 (161 views)
Permalink

[perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Tue May 22 21:42:23 2012, dmcbride [at] cpan 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

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112990

dmcbride at cpan

May 26, 2012, 10:43 PM

Post #10 of 15 (163 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Saturday May 26 2012 11:16:26 AM you wrote:
> On Tue May 22 21:42:23 2012, dmcbride [at] cpan 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.

Attachments:

signature.asc (0.19 KB)

perlbug-followup at perl

May 26, 2012, 10:49 PM

Post #11 of 15 (160 views)
Permalink

[perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Sat May 26 22:44:13 2012, dmcbride [at] cpan wrote:
> On Saturday May 26 2012 11:16:26 AM you wrote:
> > On Tue May 22 21:42:23 2012, dmcbride [at] cpan 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

---
via perlbug: queue: perl5 status: open
https://rt.perl.org:443/rt3/Ticket/Display.html?id=112990

hv at crypt

May 27, 2012, 1:13 AM

Post #12 of 15 (163 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

Darin McBride <dmcbride [at] cpan> wrote:
:On Saturday May 26 2012 11:16:26 AM [Father Chrysostomos] wrote:
:> On Tue May 22 21:42:23 2012, dmcbride [at] cpan 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

hv at crypt

May 28, 2012, 12:56 AM

Post #13 of 15 (160 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

Darin McBride <dmcbride [at] cpan> wrote:
:On Sunday May 27 2012 9:13:28 AM hv [at] crypt 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

dmcbride at cpan

Jun 23, 2012, 6:13 AM

Post #14 of 15 (148 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

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.

Attachments:

signature.asc (0.19 KB)

dmcbride at cpan

Jun 25, 2012, 10:56 AM

Post #15 of 15 (143 views)
Permalink

Re: [perl #112990] Documentation for 'kill' should call more attention to using names instead of numbers [In reply to]

On Saturday June 23 2012 9:30:32 AM you wrote:
> On Sat Jun 23 06:13:57 2012, dmcbride [at] cpan 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.

Attachments:

signature.asc (0.19 KB)

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads