[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
On 26/04/10 13:38, Richard W.M. Jones wrote:
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 $?.
Yeah. That could probably do with a comment.
+ 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 $?.
Actually I had no specific evidence that it did, I was just trying to cover bases. Do you think this is unnecessary? Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
On Mon, Apr 26, 2010 at 01:58:01PM +0100, Matthew Booth wrote:
On 26/04/10 13:38, Richard W.M. Jones wrote:
On Mon, Apr 26, 2010 at 12:04:23PM +0100, Matthew Booth wrote:
$g = undef; + + $? ||= $retval;
I'm a bit confused by this though. AFAICT the Guestfs::DESTROY function shouldn't change $?.
Actually I had no specific evidence that it did, I was just trying to cover bases. Do you think this is unnecessary?
It doesn't really matter, I was just pointing out that it won't set $? right now. Closing the handle does kill of the qemu subprocess, and so in theory it could set $? to give us the status from the subprocess, although that's unlikely to ever happen (no one cares about the status of qemu and there's no way to get it from the current C API). Add a comment? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://et.redhat.com/~rjones/virt-df/
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.
On 26/04/10 14:44, Jim Meyering wrote:
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.
Thanks, both. Pushed with extra comments. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
participants (3)
-
Jim Meyering -
Matthew Booth -
Richard W.M. Jones