chiark / gitweb /
Subprocess error handling: Initialise $? to -1
authorIan Jackson <ijackson@chiark.greenend.org.uk>
Sat, 16 Jul 2016 22:10:27 +0000 (23:10 +0100)
committerIan Jackson <ijackson@chiark.greenend.org.uk>
Sun, 17 Jul 2016 12:46:14 +0000 (13:46 +0100)
When system(3perl) fails due to syscall error, it sets only $!.  When
it succeeds it sets only $? and sometimes trashes $!.  Conversely,
close of a popened filehandle always sets both in all cases.

Document this in a comment.

So when using system and relying on $?/$! (rather than looking at
system's return value), such as when about to use failedcmd, it's
necessary to initialise $? to -1.

Fix the three call sites where system might be followed by failedcmd
but this wasn't done.

Debian/Dgit.pm
dgit

index 2555812..aa0c5a3 100644 (file)
@@ -186,8 +186,19 @@ sub waitstatusmsg () {
 }
 
 sub failedcmd {
 }
 
 sub failedcmd {
+    # Expects $!,$? as set by close - see below.
+    # To use with system(), set $?=-1 first.
+    #
+    # Actual behaviour of perl operations:
+    #   success              $!==0       $?==0       close of piped open
+    #   program failed       $!==0       $? >0       close of piped open
+    #   syscall failure      $! >0       $?=-1       close of piped open
+    #   failure              $! >0       unchanged   close of something else
+    #   success              trashed     $?==0       system
+    #   program failed       trashed     $? >0       system
+    #   syscall failure      $! >0       unchanged   system
     { local ($!); printcmd \*STDERR, _us().": failed command:", @_ or die $!; };
     { local ($!); printcmd \*STDERR, _us().": failed command:", @_ or die $!; };
-    if ($!) {
+    if ($? < 0) {
        fail "failed to fork/exec: $!";
     } elsif ($?) {
        fail "subprocess ".waitstatusmsg();
        fail "failed to fork/exec: $!";
     } elsif ($?) {
        fail "subprocess ".waitstatusmsg();
diff --git a/dgit b/dgit
index 54a1c15..1007d81 100755 (executable)
--- a/dgit
+++ b/dgit
@@ -398,7 +398,7 @@ our ($dscdata,$dscurl,$dsc,$dsc_checked,$skew_warning_vsn);
 
 sub runcmd {
     debugcmd "+",@_;
 
 sub runcmd {
     debugcmd "+",@_;
-    $!=0; $?=0;
+    $!=0; $?=-1;
     failedcmd @_ if system @_;
 }
 
     failedcmd @_ if system @_;
 }
 
@@ -1782,9 +1782,9 @@ sub check_not_dirty () {
 
     my @cmd = (@git, qw(diff --quiet HEAD));
     debugcmd "+",@cmd;
 
     my @cmd = (@git, qw(diff --quiet HEAD));
     debugcmd "+",@cmd;
-    $!=0; $?=0; system @cmd;
-    return if !$! && !$?;
-    if (!$! && $?==256) {
+    $!=0; $?=-1; system @cmd;
+    return if !$?;
+    if ($?==256) {
        fail "working tree is dirty (does not match HEAD)";
     } else {
        failedcmd @cmd;
        fail "working tree is dirty (does not match HEAD)";
     } else {
        failedcmd @cmd;
@@ -2015,7 +2015,7 @@ END
     my $diffopt = $debuglevel>0 ? '--exit-code' : '--quiet';
     my @diffcmd = (@git, qw(diff), $diffopt, $tree);
     debugcmd "+",@diffcmd;
     my $diffopt = $debuglevel>0 ? '--exit-code' : '--quiet';
     my @diffcmd = (@git, qw(diff), $diffopt, $tree);
     debugcmd "+",@diffcmd;
-    $!=0; $?=0;
+    $!=0; $?=-1;
     my $r = system @diffcmd;
     if ($r) {
        if ($r==256) {
     my $r = system @diffcmd;
     if ($r) {
        if ($r==256) {