Libguestfs - Apr 2010 - [PATCH] Fix virt-v2v exit codes

If something die()s, the error status can get lost in cleanup when various
child processes exit and overwrite $?. Specifically, the RHEV target uses a
child process for NFS access, and libguestfs launches a qemu child process.

This change ensures that DESTROY and END blocks which perform operations on
child processes explicitly preserve $?.
---
 lib/Sys/VirtV2V/Target/RHEV.pm |   10 ++++++++++
 v2v/virt-v2v.pl                |    7 +++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/lib/Sys/VirtV2V/Target/RHEV.pm b/lib/Sys/VirtV2V/Target/RHEV.pm
index f14c54f..445893a 100644
--- a/lib/Sys/VirtV2V/Target/RHEV.pm
+++ b/lib/Sys/VirtV2V/Target/RHEV.pm
@@ -143,11 +143,16 @@ sub DESTROY
 {
     my $self = shift;
 
+    my $retval = $?;
+
     # Make certain the child process dies with the object
     if (defined($self->{pid})) {
         kill(9, $self->{pid});
         waitpid($self->{pid}, WNOHANG);
+        $retval ||= $?;
     }
+
+    $? = $retval;
 }
 
 package Sys::VirtV2V::Target::RHEV::Vol;
@@ -441,6 +446,8 @@ sub DESTROY
 {
     my $self = shift;
 
+    my $retval = $?;
+
     my $eh = Sys::VirtV2V::ExecHelper->run('umount',
$self->{mountdir});
     if ($eh->status() != 0) {
         print STDERR user_message(__x("Failed to unmount {path}. Command
".
@@ -449,6 +456,7 @@ sub DESTROY
                                       path => $self->{domain_path},
                                       status => $eh->status(),
                                       output => $eh->output()));
+        $retval ||= $eh->status();
     }
 
     rmdir($self->{mountdir})
@@ -456,6 +464,8 @@ sub DESTROY
                                          "{dir}: {error}",
                                          dir => $self->{mountdir},
                                          error => $!));
+
+    $? = $retval;
 }
 
 =item create_volume(name, size)
diff --git a/v2v/virt-v2v.pl b/v2v/virt-v2v.pl
index 15d0d5b..7951303 100755
--- a/v2v/virt-v2v.pl
+++ b/v2v/virt-v2v.pl
@@ -350,6 +350,9 @@ exit(0);
 # We should always attempt to shut down the guest gracefully
 END {
     close_guest_handle();
+
+    # die() sets $? to 255, which is untidy.
+    $? = $? == 255 ? 1 : $?;
 }
 
 ###############################################################################
@@ -364,10 +367,14 @@ sub close_guest_handle
         $g->umount_all();
         $g->sync();
 
+        my $retval = $?
+
         # Note that this undef is what actually causes the underlying handle to
         # be closed. This is required to allow the RHEV target's temporary
mount
         # directory to be unmounted and deleted prior to exit.
         $g = undef;
+
+        $? ||= $retval;
     }
 }
 
-- 
1.6.6.1

On Mon, Apr 26, 2010 at 12:04:23PM +0100, Matthew Booth
wrote:
>      # Make certain the child process dies with the object
>      if (defined($self->{pid})) {
>          kill(9, $self->{pid});
>          waitpid($self->{pid}, WNOHANG);
> +        $retval ||= $?;
So the non-obvious reason why this works is because waitpid returns
the child status in $?.
> +        my $retval = $?
> +
>          # Note that this undef is what actually causes the underlying
handle to
>          # be closed. This is required to allow the RHEV target's
temporary mount
>          # directory to be unmounted and deleted prior to exit.
>          $g = undef;
> +
> +        $? ||= $retval;
I'm a bit confused by this though.  AFAICT the Guestfs::DESTROY
function shouldn't change $?.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v

Matthew Booth wrote:
> If something die()s, the error status can get lost in cleanup when various
> child processes exit and overwrite $?. Specifically, the RHEV target uses a
> child process for NFS access, and libguestfs launches a qemu child process.
>
> This change ensures that DESTROY and END blocks which perform operations on
> child processes explicitly preserve $?.
Hi Matt,
Those change look fine to me.