Libguestfs - Jun 2010 - [PATCH] Improve cleanup of libguestfs handle with Sys::VirtV2V::GuestfsHandle

This change replaces all direct usage of a Sys::Guestfs object in virt-v2v with
a Sys::VirtV2V::GuestfsHandle proxy object. The proxy does 3 things:

* Holds handle initialisation code
* Adds the ability to explicitly close the handle
* Adds the ability to register a callback to be executed before close

The cleanup code in Sys::VirtV2V::Config is updated to use the new callback
mechanism.

Additionally, the initialisation code improves handling of failure while
virt-v2v is running with uid:gid == 36:36. If cleanup handlers are called in
this context they can fail due to insufficient permissions. If this code dies,
permissions will be restored before the die is propagated. If this code receives
a user signal it will be postponed until after the code has completed.
---
 MANIFEST                         |    1 +
 lib/Sys/VirtV2V/Config.pm        |   22 +---
 lib/Sys/VirtV2V/GuestfsHandle.pm |  208 ++++++++++++++++++++++++++++++++++++++
 v2v/virt-v2v.pl                  |   72 ++------------
 4 files changed, 223 insertions(+), 80 deletions(-)
 create mode 100644 lib/Sys/VirtV2V/GuestfsHandle.pm

diff --git a/MANIFEST b/MANIFEST
index 06df2aa..fb6a2fc 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -4,6 +4,7 @@ COPYING
 COPYING.LIB
 lib/Sys/VirtV2V.pm
 lib/Sys/VirtV2V/ExecHelper.pm
+lib/Sys/VirtV2V/GuestfsHandle.pm
 lib/Sys/VirtV2V/GuestOS.pm
 lib/Sys/VirtV2V/GuestOS/RedHat.pm
 lib/Sys/VirtV2V/Config.pm
diff --git a/lib/Sys/VirtV2V/Config.pm b/lib/Sys/VirtV2V/Config.pm
index 3f689e0..761bee5 100644
--- a/lib/Sys/VirtV2V/Config.pm
+++ b/lib/Sys/VirtV2V/Config.pm
@@ -233,10 +233,13 @@ sub get_transfer_path
             my $transfer = $devices[$#devices];
 
             $g->mount_ro($transfer, $self->{transfer_mount});
-            $self->{transfer_mounted} = 1;
 
-            # We'll need this to unmount in DESTROY
-            $self->{g} = $g;
+            # Umount and remove the transfer mount point before the guestfs
+            # handle is closed
+            $g->add_on_close(sub {
+                $g->umount($self->{transfer_mount});
+                $g->rmdir($self->{transfer_mount});
+            });
         }
     }
 
@@ -498,19 +501,6 @@ sub set_default_net_mapping
     $self->{default_net_mapping} = [ $name, $type ];
 }
 
-sub DESTROY
-{
-    my $self = shift;
-
-    my $g = $self->{g};
-
-    # Remove the transfer mount point if it was used
-    $g->umount($self->{transfer_mount})
-        if(defined($self->{transfer_mounted}));
-    $g->rmdir($self->{transfer_mount})
-        if(defined($self->{transfer_mount}));
-}
-
 =back
 
 =head1 COPYRIGHT
diff --git a/lib/Sys/VirtV2V/GuestfsHandle.pm b/lib/Sys/VirtV2V/GuestfsHandle.pm
new file mode 100644
index 0000000..8720744
--- /dev/null
+++ b/lib/Sys/VirtV2V/GuestfsHandle.pm
@@ -0,0 +1,208 @@
+# Sys::VirtV2V::GuestfsHandle
+# Copyright (C) 2010 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+package Sys::VirtV2V::GuestfsHandle;
+
+use strict;
+use warnings;
+
+use Carp;
+
+use Sys::Guestfs::Lib qw(open_guest);
+use Sys::VirtV2V::UserMessage qw(user_message);
+
+use Locale::TextDomain 'virt-v2v';
+
+=pod
+
+=head1 NAME
+
+Sys::VirtV2V::GuestfsHandle - Proxy Sys::Guestfs with custom close behaviour
+
+=head1 SYNOPSIS
+
+ use Sys::VirtV2V::GuestfsHandle;
+
+ my $g = new Sys::VirtV2V::GuestfsHandle($storage, $transferiso);
+
+ # GuestfsHandle proxies all Sys::Guestfs methods
+ print join("\n", $g->list_devices());
+
+ # GuestfsHandle adds 2 new methods
+ $g->add_on_close(sub { print "Bye!\n"; });
+ $g->close();
+
+=head1 DESCRIPTION
+
+Sys::VirtV2V::GuestfsHandle is a proxy to Sys::Guestfs which adds a custom
+close() method, and the ability to register pre-close callbacks.
+
+=head1 METHODS
+
+=over
+
+=item new(storage, transferiso, isrhev)
+
+Create a new object. Open a new Sys::Guestfs handle to proxy, using the disks
+defined in the array I<storage>. Add I<transferiso> as a read-only
drive if it
+is given. If I<isrhev> is true, the handle will use user and group 36:36.
+
+=cut
+
+sub new
+{
+    my $class = shift;
+    my ($storage, $transfer, $isrhev) = @_;
+
+    my $self = {};
+
+    my $interface = "ide";
+
+    # Don't respond to signals while we're running setuid. Cleanup
operations
+    # can fail if they run as the wrong user.
+    my $sigint  = $SIG{'INT'};
+    my $sigquit = $SIG{'QUIT'};
+    my $sig_received;
+
+    $SIG{'INT'} = $SIG{'QUIT'} = sub {
+        $sig_received = shift;
+    };
+
+    if ($isrhev) {
+        $) = "36 36";
+        $> = "36";
+    }
+
+    # Open a guest handle
+    my $g;
+
+    eval {
+        $g = open_guest($storage, rw => 1, interface => $interface);
+
+        # Add the transfer iso if there is one
+        $g->add_drive_ro_with_if($transfer, $interface) if
(defined($transfer));
+
+        $g->launch();
+    };
+    my $err = $@;
+
+    if ($isrhev) {
+        $) = "0 0";
+        $> = "0";
+    }
+
+    die($err) if ($err);
+
+    if (defined($sig_received)) {
+        &$sigint($sig_received)  if ($sig_received eq 'INT');
+        &$sigquit($sig_received) if ($sig_received eq 'QUIT');
+    }
+    $SIG{'INT'}  = $sigint;
+    $SIG{'QUIT'} = $sigquit;
+
+    # Enable autosync to defend against data corruption on unclean shutdown
+    $g->set_autosync(1);
+
+    $self->{g} = $g;
+
+    $self->{onclose} = [];
+
+    bless($self, $class);
+    return $self;
+}
+
+=item add_on_close
+
+Register a callback to be called before closing the underlying Sys::Guestfs
+handle.
+
+=cut
+
+sub add_on_close
+{
+    my $self = shift;
+
+    push(@{$self->{onclose}}, shift);
+}
+
+=item close
+
+Call all registered close callbacks, then close the Sys::Guestfs handle.
+
+=cut
+
+sub close
+{
+    my $self = shift;
+
+    my $g = $self->{g};
+
+    # Nothing to do if handle is already closed
+    return unless (defined($g));
+
+    foreach my $onclose (@{$self->{onclose}}) {
+        &$onclose();
+    }
+
+    my $retval = $?;
+    $? = 0;
+
+    # This will close the underlying libguestfs handle, which may affect $?
+    $self->{g} = undef;
+
+    warn(user_message(__x("libguestfs did not shut down cleanly")))
+        if ($? != 0);
+
+    $? = $retval;
+}
+
+our $AUTOLOAD;
+sub AUTOLOAD
+{
+    (my $methodname) = $AUTOLOAD =~ m/.*::(\w+)$/;
+
+    # We don't want to call DESTROY explicitly
+    return if ($methodname eq "DESTROY");
+
+    my $self = shift;
+    my $g = $self->{g};
+
+    croak("$methodname called on guestfs handle after handle was
closed")
+        unless (defined($g));
+
+    return $g->$methodname(@_);
+}
+
+
+=back
+
+=head1 COPYRIGHT
+
+Copyright (C) 2010 Red Hat Inc.
+
+=head1 LICENSE
+
+Please see the file COPYING.LIB for the full license.
+
+=head1 SEE ALSO
+
+L<virt-v2v(1)>,
+L<http://libguestfs.org/>.
+
+=cut
+
+1;
diff --git a/v2v/virt-v2v.pl b/v2v/virt-v2v.pl
index 9008ad6..adaa9a9 100755
--- a/v2v/virt-v2v.pl
+++ b/v2v/virt-v2v.pl
@@ -25,7 +25,7 @@ use Getopt::Long;
 use Locale::TextDomain 'virt-v2v';
 
 use Sys::Guestfs;
-use Sys::Guestfs::Lib qw(open_guest get_partitions inspect_all_partitions
+use Sys::Guestfs::Lib qw(get_partitions inspect_all_partitions
                          inspect_operating_systems mount_operating_system
                          inspect_in_detail);
 
@@ -37,6 +37,7 @@ use Sys::VirtV2V::Connection::LibVirtXML;
 use Sys::VirtV2V::Target::LibVirt;
 use Sys::VirtV2V::Target::RHEV;
 use Sys::VirtV2V::ExecHelper;
+use Sys::VirtV2V::GuestfsHandle;
 use Sys::VirtV2V::GuestOS;
 use Sys::VirtV2V::UserMessage qw(user_message);
 
@@ -357,18 +358,9 @@ my $storage = $conn->get_storage_paths();
 my $transferiso;
 $transferiso = $config->get_transfer_iso();
 
-if ($output_method eq 'rhev') {
-    $) = "36 36";
-    $> = "36";
-}
-
 # Open a libguestfs handle on the guest's storage devices
-my $g = get_guestfs_handle($storage, $transferiso);
-
-if ($output_method eq 'rhev') {
-    $) = "0";
-    $> = "0";
-}
+my $g = new Sys::VirtV2V::GuestfsHandle($storage, $transferiso,
+                                        $output_method eq 'rhev');
 
 my $os;
 my $guestcaps;
@@ -390,11 +382,11 @@ eval {
 # can result in failure to umount RHEV export's temporary mount point.
 if ($@) {
     my $err = $@;
-    close_guest_handle();
+    $g->close();
     die($err);
 }
 
-close_guest_handle();
+$g->close();
 
 $target->create_guest($os, $dom, $guestcaps);
 
@@ -418,58 +410,10 @@ END {
 ###############################################################################
 ## Helper functions
 
-sub close_guest_handle
-{
-    # Config may cache the guestfs handle, preventing cleanup below
-    $config = undef;
-
-    if (defined($g)) {
-        my $retval = $?;
-
-        eval {
-            $g->umount_all();
-            $g->sync();
-        };
-        if ($@) {
-            warn(user_message(__x("Error cleaning up guest handle:
{error}",
-                                  error => $@)));
-            $retval ||= 1;
-        }
-
-        # 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;
-
-        # The above undef causes libguestfs's qemu process to be killed.
This
-        # may update $?, so we preserve it here.
-        $? ||= $retval;
-    }
-}
-
 sub signal_exit
 {
-    close_guest_handle();
-    exit(1);
-}
-
-sub get_guestfs_handle
-{
-    my ($storage, $transferiso) = @_;
-    my $interface = "ide";
-
-    my $g = open_guest($storage, rw => 1, interface => $interface);
-
-    # Add the transfer iso if there is one
-    $g->add_drive_ro_with_if($transferiso, $interface)
-        if(defined($transferiso));
-
-    # Enable autosync to defend against data corruption on unclean shutdown
-    $g->set_autosync(1);
-
-    $g->launch();
-
-    return $g;
+    $g->close() if (defined($g));
+    die(user_message(__x("Received signal {sig}. Exiting.", sig =>
shift)));
 }
 
 # Inspect the guest's storage. Returns an OS hashref as returned by
-- 
1.7.1

On 17/06/10 17:05, Matthew Booth wrote:
> This change replaces all direct usage of a Sys::Guestfs object in virt-v2v
with
> a Sys::VirtV2V::GuestfsHandle proxy object. The proxy does 3 things:
>
> * Holds handle initialisation code
> * Adds the ability to explicitly close the handle
> * Adds the ability to register a callback to be executed before close
>
> The cleanup code in Sys::VirtV2V::Config is updated to use the new callback
> mechanism.
>
> Additionally, the initialisation code improves handling of failure while
> virt-v2v is running with uid:gid == 36:36. If cleanup handlers are called
in
> this context they can fail due to insufficient permissions. If this code
dies,
> permissions will be restored before the die is propagated. If this code
receives
> a user signal it will be postponed until after the code has completed.
I should have mentioned here that this fixes RHBZ#596071 and 
RHBZ#596015. I've updated my local commit.

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 Thu, Jun 17, 2010 at 05:05:09PM +0100, Matthew Booth
wrote:
> This change replaces all direct usage of a Sys::Guestfs object in virt-v2v
with
> a Sys::VirtV2V::GuestfsHandle proxy object. The proxy does 3 things:
> 
> * Holds handle initialisation code
> * Adds the ability to explicitly close the handle
> * Adds the ability to register a callback to be executed before close
> 
> The cleanup code in Sys::VirtV2V::Config is updated to use the new callback
> mechanism.
> 
> Additionally, the initialisation code improves handling of failure while
> virt-v2v is running with uid:gid == 36:36. If cleanup handlers are called
in
> this context they can fail due to insufficient permissions. If this code
dies,
> permissions will be restored before the die is propagated. If this code
receives
> a user signal it will be postponed until after the code has completed.
This all seems reasonable.  I think the only thing I might have
changed would be to try a subclass, but the proxy / AUTOLOAD method
should also work fine.

ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://et.redhat.com/~rjones/libguestfs/
See what it can do: http://et.redhat.com/~rjones/libguestfs/recipes.html