Re: [PATCH] perlipc.pod Revamp

OK, now replying in detail.

On Monday 01 November 2010 19:49:54 Tom Christiansen wrote:
> Thank you for your edits.  I agree with most of them.
> 
> A lot of these are just personal style things.  It's nearly not worth
> commenting on.  My own style *has* gotten more regular since I wrote that,
> but not much looks too seriously different from how I'd write things today.
> This is mostly quibbling, although there are a few genuine items that
> reauire addressing.
> 
> I haven't read the replies yet.
> 
> >+    our $shucks = 0;
> 
> I'm always nervous about initializing a global through our().
> You should only do so once, but nothing guarantees that you do.
> I'd never such in an example, because it's too fragile.
> 

Well, without this our declaration, the example won't pass "use strict;", so I 
had to add it. Would "my" be better here? "my" should work I think But I 
wasn't sure about it.

> >     sub catch_zap {
> >     
> >         my $signame = shift;
> >
> >+
> >
> >         $shucks++;
> >
> >+
> >
> >         die "Somebody sent me a SIG$signame";
> >     
> >     }
> >
> >+
> >
> >     $SIG{INT} = 'catch_zap';  # could fail in modules
> >     $SIG{INT} = \&catch_zap;  # best strategy
> 
> I don't much mind the vertical whitespace following the function,
> but I see no reason for it within the function.  The more
> vertical whitespace, the more you dilute the code density of what
> can be immediately apprehended at a glance.  Sometimes you want
> to do that, but sometimes you want *not* to do it.
> 
> Remembering that these code snippets need to fit comfortably on a
> manpage, which is a paged document, I suspect that this case
> belongs to the latter group, not to the former.
> 

Very well, I removed it in my copy on github.

> >@@ -45,11 +48,21 @@ system, or you can retrieve them from the Config
> >module. Set up an
> >
> > indexed by name to get the number:
> >     use Config;
> >
> >-    defined $Config{sig_name} || die "No sigs?";
> >-    foreach $name (split(' ', $Config{sig_name})) {
> >-        $signo{$name} = $i;
> >-        $signame[$i] = $name;
> >-        $i++;
> >+
> >+    if (!defined $Config{sig_name})
> >+    {
> 
> See, more spurious vertical whitespace.  If you must make this
> a block, it should be clustered.  And I suspect that boolean
> truth would suffice.

OK, I'll reconsider.

> 
> >+        die "No sigs?";
> >+    }
> >+
> >+    my (%signo, @signame);
> >+
> >+    my $index = 0;
> >+
> >+    foreach my $name (split(' ', $Config{sig_name})) {
> 
> I never write "foreach my" because my brain never inserts the
> English word "of" before the "my"; I always write "for my".

I don't have a problem with that (People say "for each child in my family", 
etc.) and generally the Perl custom is to use foreach for iterating over a 
list, and "for" for either C-style for-loops or iterating over a range.

> 
> >+        $signo{$name} = $index;
> >+        $signame[$index] = $name;
> 
> These days I would work harder to vertically align the equal signs
> between adjacent assignments.

Fixed.

> 
> >+
> 
> More diluting vertical whitespace.
> 
> >+        $index++;
> >
> >     }
> > 
> > So to check whether signal 17 and SIGALRM were the same, do just this:
> >@@ -79,8 +92,9 @@ values are "inherited" by functions called from within
> >that block.)
> >
> >     sub precious {
> >     
> >         local $SIG{INT} = 'IGNORE';
> 
> I'd now write that "IGNORE".  My current *personal* style is to
> use double quotes on all strings I am not trying to have *not*
> interpolate.  I find it easier to read because the glyph is
> wider, so can't ever look like a backquote no matter the font.

Hmmm. Damian's PBP recommends using single-quotes for strings that don't have 
interpolated variables/etc., and double quotes when you need interpolation and 
other features of ""/qq{}.

> >     sub more_functions {
> >     
> >         # interrupts still ignored, for now...
> 
> I might say SIGINTS instead of interrupts, which I
> tend to think of as hardware things.

Fixed in my copy.

> 
> >     }
> >
> >@@ -116,7 +130,7 @@ You may be able to determine the cause of failure
> >using C<%!>.
> >
> > You might also want to employ anonymous functions for simple signal
> >
> > handlers:
> >-    $SIG{INT} = sub { die "\nOutta here!\n" };
> >+    $SIG{INT} = sub { die "\nOutta here!\n"; };
> 
> Oh, no.  I would no more put a semicolon at the end of a statement whose
> close brace immediately follows on the same line than I would put a comma
> after the last list element right before a close paren on the same line,
> and for just the same reason.
> 
> I would also not never omit them if the closing punctuation is on
> a separate line.

Well, this is bikeshedding. I think always adding a semicolon at the end of 
every statement is good style, and is safer if you add stuff later on. PBP 
recommends it, I believe.

> >+
> >
> >         cleanup_child($pid, $?);
> >     
> >     };
> 
> That's a great deal of vertical whitespace.  I can't see what
> it usefully adds.  It looks like double-spaced text.
> 

Removed to make the example shorter.

> >+
> >+            $children{$pid}=1;
> >
> >             # ...
> >             system($command);
> >             # ...
> >
> >+
> 
> Why the vertical whitespace here?  Why swap the other
> "# ..."  with a blank line, but not these?
> 

I swapped it with the comment "I'm the child - do something".

> >        }
> >     
> >     }
> >
> >@@ -199,12 +226,16 @@ using longjmp() or throw() in other languages.
> >
> > Here's an example:
> >     eval {
> >
> >-        local $SIG{ALRM} = sub { die "alarm clock restart" };
> >+
> >+        local $SIG{ALRM} = sub { die "alarm clock restart"; };
> 
> No new semicolon.
> 

OK, who gets to veto this? 

> > If the operation being timed out is system() or qx(), this technique
> > is liable to generate zombies.    If this matters to you, you'll
> >
> >@@ -246,16 +277,18 @@ info to show that it works and should be replaced
> >with the real code.
> >
> >   $|=1;
> 
> Spaces around the equals sign.
> 
Done.

> >       my $c = 0;
> >       while (++$c) {
> >           sleep 2;
> >           print "$c\n";
> 
> I'm not proud of having used a signal letter for a variable name.
> In fact, I don't care for it here.

Changed to $count.

I integrated all your commentary into:

[code]
    sub _create_named_pipe
    {
        local $ENV{PATH} = $ENV{PATH}
            . join "", map { ":$_" } qw(/etc /usr/etc /sbin/ /usr/sbin);

        if (system('mknod', $path, 'p') != 0) {
            if (system('mkfifo', $path) != 0) {
                die "mk{nod,fifo} $path failed - $?";
            }
        }

        # Return success.
        return 1;
    }
[/code]

The brace on a separate line is my own personal style and does not belong in 
perl*.pod.

> >+            }
> >+        }
> >+
> >+        # Return success.
> >+        return 1;
> >
> >     }
> >
> >@@ -313,22 +357,26 @@ from that file, the reading program will block and
> >your program will
> >
> > supply the new signature.  We'll use the pipe-checking file test B<-p>
> > to find out whether anyone (or anything) has accidentally removed our
> > fifo.
> >
> >-    chdir; # go home
> >-    $FIFO = '.signature';
> >+    use POSIX qw();
> >+
> >+    chdir; # Go home
> >+    my $fifo_filename = '.signature';
> >
> >     while (1) {
> >
> >-        unless (-p $FIFO) {
> >-            unlink $FIFO;
> >-            require POSIX;
> >-            POSIX::mkfifo($FIFO, 0700)
> >-                or die "can't mkfifo $FIFO: $!";
> 
> The goal was not to load the POSIX module unconditionally.
> That has been lost.
>

Why should we not load POSIX always? Is this a micro-optimisation?

> >-        print FIFO "John Smith (smith\@host.org)\n", `fortune -s`;
> >-        close FIFO;
> >-        sleep 2;    # to avoid dup signals
> >+        # Next line blocks until there's a reader.
> >+        open (my $fifo_fh, ">", $fifo_filename)
> >+            or die "can't write ${fifo_filename}: $!";
> >+        print {$fifo_fh} "John Smith (smith\@host.org)\n", `fortune -s`;
> 
> Why in the world has that become a dative block?
> 

PBP recommends (and I agree) against writing

print $fifo_fh "Hello\n";

Because it is too easy to confuse with:

print $fifo_fh, "Hello\n";

And recommends writing:

print {$fifo_fh} "Hello\n";

instead.

> >+        close $fifo_fh;
> 
> That should test its return value.
> 
> 	close($fifo_fh)	    || die "can't close $fifo_fh:

That would interpolate $fifo_fh there.

I changed it to:

close($fifo_fh) or die "can't close \$fifo_fh: $?";

> >+
> >+        sleep 2;    # To avoid duplicate signals
> 
> Wish I knew what I had been thinking.
>

Should I just remove it?
 
> >-N.B. If a signal of any given type fires multiple times during an opcode
> >+N.B.: if a signal of any given type fires multiple times during an opcode
> 
> I'd try to find something less Latinate.  Plus the style guide says not
> to start sentence with "note that", anyway.
> 

Changed to "One should note that"

> > (such as from a fine-grained timer), the handler for that signal will
> > only be called once after the opcode completes, and all the other
> > instances will be discarded.  Furthermore, if your system's signal queue
> >
> >@@ -397,7 +445,7 @@ breaks into IO operations like C<read> (used to
> >implement Perls
> >
> > E<lt>E<gt> operator). On older Perls the handler was called
> 
> I'd write that as C<< <> >> now.

Done, thanks.

> 
> > immediately (and as C<read> is not "unsafe" this worked well). With
> > the "deferred" scheme the handler is not called immediately, and if
> >
> >-Perl is using system's C<stdio> library that library may re-start the
> >+Perl is using the system's C<stdio> library, that library may re-start
> >the
> 
> I don't thikn I'd hyphenate restart.
> 

Hyphen removed.

> > =back
> >
> >@@ -470,24 +519,27 @@ C<"unsafe"> (a new feature since Perl 5.8.1).
> >
> > Perl's basic open() statement can also be used for unidirectional
> 
> It's not a statement.  It's a function.

Changed.

> 
> > interprocess communication by either appending or prepending a pipe
> >
> >-symbol to the second argument to open().  Here's how to start
> >-something up in a child process you intend to write to:
> >+symbol to the second argument to open(), or by using the three-args form
> >+with the C<"|-"> or C<"-|"> modes (which won't work on Windows systems.)
> >+. Here's how to start something up in a child process you intend to write
> >to:
> >
> >-    open(SPOOLER, "| cat -v | lpr -h 2>/dev/null")
> >-                    || die "can't fork: $!";
> >+    open (my $spooler, "| cat -v | lpr -h 2>/dev/null")
> >+                    or die "can't fork: $!";
> 
> That's unnecessary.

What is and why?

> >+    close ($spooler) or die "bad spool: $! $?";
> 
> That should be
> 
>     close($spooler)	 || die "close to spooler pipe failed: $? $!";
> 
> But that's still awkward.



> 
> > And here's how to start up a child process you intend to read from:
> >-    open(STATUS, "netstat -an 2>&1 |")
> >-                    || die "can't fork: $!";
> >-    while (<STATUS>) {
> >+    open (my $status, "netstat -an 2>&1 |")
> >+                    or die "can't fork: $!";
> 
> Please don't make it all look like the same stuff.  The "||"
> is used for a reason.

Why do you find it preferable over "or"? "or" has an ultra-low precendance and 
so is harder to misuse.

> 
> >+
> >+    while (<$status>) {
> >
> >         next if /^(tcp|udp)/;
> >         print;
> >     
> >     }
> >
> >-    close STATUS || die "bad netstat: $! $?";
> 
> That's actually a bad thing.  Looks like a perl4 vestige.
> 
> >+
> >+    close ($status) or die "bad netstat: $! $?";
> 
> No space.
> 
>     close($status) 	 || die "bad netstat: $! $?";
> 
> All the || should line up to the same column in that example.
> That's why it's strangely indented earlier.
> 

More bikeshedding.

> > =head2 Filehandles
> >
> >@@ -546,7 +598,8 @@ child process cannot outlive the parent.
> >
> > =head2 Background Processes
> >
> >-You can run a command in the background with:
> >+Assuming your shell supports that, you can run a command in the
> >background
> >
> >+with:
> >     system("cmd &");
> >
> >@@ -569,8 +622,8 @@ output doesn't wind up on the user's terminal).
> >
> >     sub daemonize {
> >     
> >         chdir '/'                      or die "Can't chdir to /: $!";
> >
> >-        open STDIN, '/dev/null'        or die "Can't read /dev/null: $!";
> >-        open STDOUT, '>/dev/null'
> >+        open STDIN, '<', '/dev/null'        or die "Can't read /dev/null:
> >$!";
> 
> Nope.  Make all the || die align.  AND DO NOT OMIT THE PARENS AROUND
> THE FUNCTIon CALL!!

Fixed the alignment. There were no parentheses around open.


> >+        close($kid_to_write) or warn "kid exited $?";
> 
> No "or".  I absolutely never ever use it.  That way I never
> get confused as the recent CPAN bug report shows that people do.

Which bug report?

> 
> >     } else {     # child
> >     
> >         ($EUID, $EGID) = ($UID, $GID); # suid progs only
> >
> >-        open (FILE, "> /safe/file")
> >-            || die "can't open /safe/file: $!";
> >+        open (my $file, ">", "/safe/file")
> 
> Why?  There is 100.00000000000% chance that that will behave.
> It's a constant string of known content.

Still, we should not encourage such potentially bad idioms. Here it is 
harmless, but people can learn that it's OK to do ">$myfile" too.

> >     # add error processing as above
> >
> >-    $pid = open(KID_TO_READ, "-|");
> >+    $pid = open($kid_to_read, "-|");
> 
> Since we're getting our knickers in a snit now,
> I wonder where that was declared?

It wasn't - thanks for spotting it. Fixed now.


> >+        # NOT REACHED
> >
> >     }
> > 
> > And here's a safe pipe open for writing:
> >     # add error processing as above
> >
> >-    $pid = open(KID_TO_WRITE, "|-");
> >+    $pid = open($kid_to_write, "|-");
> >
> >     $SIG{PIPE} = sub { die "whoops, $program pipe broke" };
> >     
> >     if ($pid) {  # parent
> >     
> >         for (@data) {
> >
> >-            print KID_TO_WRITE;
> >+            print $kid_to_write;
> 
> And *HERE* is where we have a problem, Houston.  That is
> now incorrect!! This is the price of political correctness
> gone MAD!
> 
>     % perl -le '$fh = \*STDOUT; print $fh'
>     GLOB(0x7e0e6020)
> 
> SEE???  That's why you use handles that Perl knows are handles.
> 

Fixed.
> >     defined $pid or die "fork failed; $!";
> 
> Bad demicolon.  And other things.
> 
>     die "fork failed: $!"  unless defined $pid;
> 
> >     if ($pid) {
> >     
> >         if (my $sub_pid = fork()) {
> >
> >-            close WRITER;
> >+            close $writer;
> 
>     close($writer) || die ....
> 

Fixed.

> >-In the above, the true parent does not want to write to the WRITER
> 
> In the above *what*?  In the code above, I bet.
> 

Fixed.

> >     if ($pid) {
> >
> >-        close READER;
> >+        close $reader;
> >
> >         if (my $sub_pid = fork()) {
> >
> >-            close WRITER;
> >+            close $writer;
> >
> >         }
> >         else {
> >
> >-            # write to WRITER...
> >+            # write to $writer...
> >
> >             exit;
> >         
> >         }
> >
> >-        # write to WRITER...
> >+        # write to $writer...
> >
> >     }
> >     else {
> >
> >-        open STDIN, "<&READER";
> >-        close WRITER;
> >+        open STDIN, "<&", $reader;
> >+        close $writer;
> >
> >         # do something...
> >         exit;
> >     
> >     }
> >
> >@@ -733,11 +787,11 @@ open() which sets one file descriptor to another, as
> >
> >below:
> > Since Perl 5.8.0, you can also use the list form of C<open> for pipes :
> > the syntax
> >
> >-    open KID_PS, "-|", "ps", "aux" or die $!;
> >+    open my $kid_ps, "-|", "ps", "aux" or die $!;
> 
> That looks just terrible I'm sure I would never have
> written such a nasty thing.
> 
>     open(my $kid_ps, "-|", qw[ ps aux ])
> 
> 	|| die "pipe from ps failed: $!";

Corrected.

 > 


> > forks the ps(1) command (without spawning a shell, as there are more than
> > three arguments to open()), and reads its standard output via the
> >
> >-C<KID_PS> filehandle.  The corresponding syntax to write to command
> >+C<$kid_ps> filehandle.  The corresponding syntax to write to command
> >
> > pipes (with C<"|-"> in place of C<"-|">) is also implemented.
> > 
> > Note that these operations are full Unix forks, which means they may not
> > be
> >
> >@@ -775,7 +829,7 @@ While this works reasonably well for unidirectional
> >communication, what
> >
> > about bidirectional communication?  The obvious thing you'd like to do
> >
> > doesn't actually work:
> >-    open(PROG_FOR_READING_AND_WRITING, "| some program |")
> >+    open (my $prog_for_reading_and_writing, "| some program |")
> >
> > and if you forget to use the C<use warnings> pragma or the B<-w> flag,
> >
> > then you'll miss out entirely on the diagnostic message:
> >@@ -813,27 +867,32 @@ commands are designed to operate over pipes, so this
> >seldom works
> >
> > unless you yourself wrote the program on the other end of the
> > double-ended pipe.
> >
> >-A solution to this is the nonstandard F<Comm.pl> library.  It uses
> >-pseudo-ttys to make your program behave more reasonably:
> >+A solution to this is the non-core IPC-Run CPAN distribution (
> >+L<http://search.cpan.org/dist/IPC-Run/> )
> 
> How come that's not written as an IPC::Run module?
> 

What do you mean?

> >+
> >+    use IPC::Run qw(run timeout);
> >+
> >+    my ($in, $out, $err);
> >+    run ['cat', '-n'], \$in, \$out, \$err, timeout(10)
> >+        or die "Cannot run cat: $?";
> 
> That's just terrible.  Use this:
> 
>     run([qw(cat -n)], \($in, $out, $err), timeout(10))
> 
>     	|| die "couldn't run cat -n program: $?";
> 
> Better yet, use an array so you don't have the same
> info in two places:
> 
>     @cat_args = qw[cat -n];
> 
>     run(\(@cat_args, $in, $out, $err) => timeout(10))
> 
>     	|| die "couldn't run @cat_args program: $?";

Fixed to an extent.

> >
> >-    require 'Comm.pl';
> >-    $ph = open_proc('cat -n');
> >
> >     for (1..10) {
> 
> Spaces around operators.

Fixed.

> >also
> >+addresses this kind of thing, but for Unix systems only. This module
> >requires +two other modules from CPAN: IO::Pty and IO::Stty. It sets up a
> >+pseudo-terminal to interact with programs that insist on using talking to
> 
> Don't think that needs hyphenation.
> 
> Don't think using belongs there.
> 

Fixed the using. I think that pseduoterminal looks weird.

> >+the terminal device driver.  If your system is amongst those supported,
> >this
> 
> In my own speech, I more often use "among" when the following
> word begins with a consonant sound.  I'd probably say "amongst
> others" but "among those".  That's a tad precious, perhaps even
> silly: too much like "my nose and mine eyes".
> 

Fixed to among.

> >+may be your best bet.
> >
> > =head2 Bidirectional Communication with Yourself
> >
> >@@ -1095,7 +1154,7 @@ go back to service a new client.
> >
> >         if (! defined($pid = fork)) {
> >         
> >             logmsg "cannot fork: $!";
> >             return;
> >
> >-        }
> >+        }
> >
> >         elsif ($pid) {
> >         
> >             logmsg "begat $pid";
> >             return; # I'm the parent
> >
> >@@ -1125,7 +1184,7 @@ to be reported.  However the introduction of safe
> >signals (see
> >
> > L</Deferred Signals (Safe Signals)> above) in Perl 5.7.3 means that
> > accept() may also be interrupted when the process receives a signal.
> > This typically happens when one of the forked sub-processes exits and
> 
> Don't think that needs hyphenation.
> 

I think it does. Not fixed yet.

Regards,

	Shlomi Fish

-- 
-----------------------------------------------------------------
Shlomi Fish       http://www.shlomifish.org/
Why I Love Perl - http://shlom.in/joy-of-perl

<rindolf> She's a hot chick. But she smokes.
<go|dfish> She can smoke as long as she's smokin'.

Please reply to list if it's a mailing list post - http://shlom.in/reply .