Libguestfs - Jun 2010 - [PATCH] Fix cleanup of guest handle when installing with local files

Change 13412f7629133b25fda32c35fdb0276e22de3445 caused Config to keep a
reference to the guestfs handle in certain circumstances. This caused a
regression in the cleanup code because the handle was no longer closed in
close_guest_handle.

This change explicitly drops the reference to $config before closing the guest
handle. It also localises $guestos, which also keeps a reference to the guestfs
handle in an eval block.
---
 v2v/virt-v2v.pl |   38 ++++++++++++++++++++++++--------------
 1 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/v2v/virt-v2v.pl b/v2v/virt-v2v.pl
index 726cd50..9008ad6 100755
--- a/v2v/virt-v2v.pl
+++ b/v2v/virt-v2v.pl
@@ -370,16 +370,29 @@ if ($output_method eq 'rhev') {
     $> = "0";
 }
 
-# Inspect the guest
-my $os = inspect_guest($g);
+my $os;
+my $guestcaps;
+eval {
+    # Inspect the guest
+    $os = inspect_guest($g);
+
+    # Instantiate a GuestOS instance to manipulate the guest
+    my $guestos = Sys::VirtV2V::GuestOS->new($g, $os, $dom, $config);
 
-# Instantiate a GuestOS instance to manipulate the guest
-my $guestos = Sys::VirtV2V::GuestOS->new($g, $os, $dom, $config);
+    # Modify the guest and its metadata
+    $guestcaps = Sys::VirtV2V::Converter->convert($g, $guestos,
+                                                  $config, $dom, $os,
+                                                 
$conn->get_storage_devices());
+};
 
-# Modify the guest and its metadata
-my $guestcaps = Sys::VirtV2V::Converter->convert($g, $guestos,
-                                                 $config, $dom, $os,
-                                                
$conn->get_storage_devices());
+# If any of the above commands result in failure, we need to ensure that the
+# guestfs qemu process is cleaned up before further cleanup. Failure to do this
+# can result in failure to umount RHEV export's temporary mount point.
+if ($@) {
+    my $err = $@;
+    close_guest_handle();
+    die($err);
+}
 
 close_guest_handle();
 
@@ -397,11 +410,8 @@ if($guestcaps->{virtio}) {
 
 exit(0);
 
-# We should always attempt to shut down the guest gracefully
+# die() sets $? to 255, which is untidy.
 END {
-    close_guest_handle();
-
-    # die() sets $? to 255, which is untidy.
     $? = $? == 255 ? 1 : $?;
 }
 
@@ -410,8 +420,8 @@ END {
 
 sub close_guest_handle
 {
-    # Perform GuestOS cleanup before closing the handle
-    $guestos = undef;
+    # Config may cache the guestfs handle, preventing cleanup below
+    $config = undef;
 
     if (defined($g)) {
         my $retval = $?;
-- 
1.7.0.1

On Wed, Jun 09, 2010 at 05:19:09PM +0100, Matthew Booth
wrote:
> Change 13412f7629133b25fda32c35fdb0276e22de3445 caused Config to keep a
> reference to the guestfs handle in certain circumstances. This caused a
> regression in the cleanup code because the handle was no longer closed in
> close_guest_handle.
> 
> This change explicitly drops the reference to $config before closing the
guest
> handle. It also localises $guestos, which also keeps a reference to the
guestfs
> handle in an eval block.
ACK as discussed on IRC.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top