From 0a85fbc946d7838366c35c0425efddb1a91f593c Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Mon, 8 Oct 2012 17:26:53 +0100
Subject: Add support for hotplugging (removing disks). New API: remove-drive.

Note because of a bug in fuser, this only works with psmisc >= 22.20.

This also updates the hotplugging tests.
---
 daemon/hotplug.c                       | 101 +++++++++++++++++++++++++++++++--
 generator/actions.ml                   |  39 +++++++++++++
 src/MAX_PROC_NR                        |   2 +-
 src/guestfs-internal.h                 |   1 +
 src/guestfs.pod                        |  10 +++-
 src/launch-libvirt.c                   |  35 ++++++++++++
 src/launch.c                           |  54 ++++++++++++++++++
 tests/hotplug/Makefile.am              |   6 +-
 tests/hotplug/test-hot-remove.pl       |  88 ++++++++++++++++++++++++++++
 tests/hotplug/test-hotplug-repeated.pl |  57 +++++++++++++++++++
 10 files changed, 383 insertions(+), 10 deletions(-)
 create mode 100755 tests/hotplug/test-hot-remove.pl
 create mode 100755 tests/hotplug/test-hotplug-repeated.pl

diff --git a/daemon/hotplug.c b/daemon/hotplug.c
index aae638e7..05367e3b 100644
--- a/daemon/hotplug.c
+++ b/daemon/hotplug.c
@@ -29,6 +29,17 @@
 #include "actions.h"
 
 #define HOT_ADD_TIMEOUT 30 /* seconds */
+#define HOT_REMOVE_TIMEOUT HOT_ADD_TIMEOUT
+
+static void
+hotplug_error (const char *op, const char *path, const char *verb,
+               int timeout)
+{
+  reply_with_error ("%s drive: '%s' did not %s after %d seconds: "
+                    "this could mean that virtio-scsi (in qemu or kernel) "
+                    "or udev is not working",
+                    op, path, verb, timeout);
+}
 
 /* Wait for /dev/disk/guestfs/<label> to appear.  Timeout (and error)
  * if it doesn't appear after a reasonable length of time.
@@ -59,9 +70,91 @@ do_internal_hot_add_drive (const char *label)
     sleep (1);
   }
 
-  reply_with_error ("hot-add drive: '%s' did not appear after %d seconds: "
-                    "this could mean that virtio-scsi (in qemu or kernel) "
-                    "or udev is not working",
-                    path, HOT_ADD_TIMEOUT);
+  hotplug_error ("hot-add", path, "appear", HOT_ADD_TIMEOUT);
+  return -1;
+}
+
+GUESTFSD_EXT_CMD(str_fuser, fuser);
+
+/* This function is called before a drive is hot-unplugged. */
+int
+do_internal_hot_remove_drive_precheck (const char *label)
+{
+  size_t len = strlen (label);
+  char path[len+64];
+  int r;
+  char *out, *err;
+
+  /* Ensure there are no requests in flight (thanks Paolo Bonzini). */
+  udev_settle ();
+  sync_disks ();
+
+  snprintf (path, len+64, "/dev/disk/guestfs/%s", label);
+
+  r = commandr (&out, &err, str_fuser, "-v", "-m", path, NULL);
+  if (r == -1) {
+    reply_with_error ("fuser: %s: %s", path, err);
+    free (out);
+    free (err);
+    return -1;
+  }
+  free (err);
+
+  /* "fuser returns a non-zero return code if none of the specified
+   * files is accessed or in case of a fatal error. If at least one
+   * access has been found, fuser returns zero."
+   */
+  if (r == 0) {
+    reply_with_error ("disk with label '%s' is in use "
+                      "(eg. mounted or belongs to a volume group)", label);
+
+    /* Useful for debugging when a drive cannot be unplugged. */
+    if (verbose)
+      fprintf (stderr, "%s\n", out);
+
+    free (out);
+
+    return -1;
+  }
+
+  free (out);
+
+  return 0;
+}
+
+/* This function is called after a drive is hot-unplugged.  It checks
+ * that it has really gone and udev has finished processing the
+ * events, in case the user immediately hotplugs a drive with an
+ * identical label.
+ */
+int
+do_internal_hot_remove_drive (const char *label)
+{
+  time_t start_t, now_t;
+  size_t len = strlen (label);
+  char path[len+64];
+  int r;
+
+  snprintf (path, len+64, "/dev/disk/guestfs/%s", label);
+
+  time (&start_t);
+
+  while (time (&now_t) - start_t <= HOT_REMOVE_TIMEOUT) {
+    udev_settle ();
+
+    r = access (path, F_OK);
+    if (r == -1) {
+      if (errno != ENOENT) {
+        reply_with_perror ("%s", path);
+        return -1;
+      }
+      /* else udev has removed the file, so we can return */
+      return 0;
+    }
+
+    sleep (1);
+  }
+
+  hotplug_error ("hot-remove", path, "disappear", HOT_REMOVE_TIMEOUT);
   return -1;
 }
diff --git a/generator/actions.ml b/generator/actions.ml
index 39c71ab8..0c1f4ab2 100644
--- a/generator/actions.ml
+++ b/generator/actions.ml
@@ -2355,6 +2355,25 @@ backing file.
 Note that detecting disk features can be insecure under some
 circumstances.  See L<guestfs(3)/CVE-2010-3851>." };
 
+  { defaults with
+    name = "remove_drive";
+    style = RErr, [String "label"], [];
+    tests = [];
+    shortdesc = "remove a disk image";
+    longdesc = "\
+This function is conceptually the opposite of C<guestfs_add_drive_opts>.
+It removes the drive that was previously added with label C<label>.
+
+Note that in order to remove drives, you have to add them with
+labels (see the optional C<label> argument to C<guestfs_add_drive_opts>).
+If you didn't use a label, then they cannot be removed.
+
+You can call this function before or after launching the handle.
+If called after launch, if the attach-method supports it, we try to hot
+unplug the drive: see L<guestfs(3)/HOTPLUGGING>.  The disk B<must not>
+be in use (eg. mounted) when you do this.  We try to detect if the
+disk is in use and stop you from doing this." };
+
 ]
 
 (* daemon_functions are any functions which cause some action
@@ -9969,6 +9988,26 @@ are the full raw block device and partition names
     longdesc = "\
 This function is used internally when hotplugging drives." };
 
+  { defaults with
+    name = "internal_hot_remove_drive_precheck";
+    style = RErr, [String "label"], [];
+    proc_nr = Some 371;
+    in_fish = false; in_docs = false;
+    tests = [];
+    shortdesc = "internal hotplugging operation";
+    longdesc = "\
+This function is used internally when hotplugging drives." };
+
+  { defaults with
+    name = "internal_hot_remove_drive";
+    style = RErr, [String "label"], [];
+    proc_nr = Some 372;
+    in_fish = false; in_docs = false;
+    tests = [];
+    shortdesc = "internal hotplugging operation";
+    longdesc = "\
+This function is used internally when hotplugging drives." };
+
 ]
 
 (* Non-API meta-commands available only in guestfish.
diff --git a/src/MAX_PROC_NR b/src/MAX_PROC_NR
index 5b0cffbc..ba300673 100644
--- a/src/MAX_PROC_NR
+++ b/src/MAX_PROC_NR
@@ -1 +1 @@
-370
+372
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index db9818c3..273900ef 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -169,6 +169,7 @@ struct attach_ops {
 
   /* Hotplugging drives. */
   int (*hot_add_drive) (guestfs_h *g, struct drive *drv, size_t drv_index);
+  int (*hot_remove_drive) (guestfs_h *g, struct drive *drv, size_t drv_index);
 };
 extern struct attach_ops attach_ops_appliance;
 extern struct attach_ops attach_ops_libvirt;
diff --git a/src/guestfs.pod b/src/guestfs.pod
index 624e743e..0dbc30f1 100644
--- a/src/guestfs.pod
+++ b/src/guestfs.pod
@@ -606,9 +606,9 @@ download etc. instead.
 
 =head2 HOTPLUGGING
 
-In libguestfs E<ge> 1.20, you may add drives after calling
-L</guestfs_launch>.  There are some restrictions, see below.
-This is called I<hotplugging>.
+In libguestfs E<ge> 1.20, you may add drives and remove after calling
+L</guestfs_launch>.  There are some restrictions, see below.  This is
+called I<hotplugging>.
 
 Only a subset of the attach-method backends support hotplugging
 (currently only the libvirt attach-method has support).  It also
@@ -629,6 +629,10 @@ so that the newly added disk has a predictable name.  For example:
  if (guestfs_part_disk ("/dev/disk/guestfs/newdisk", "mbr") == -1)
    error ("partitioning of hot-added disk failed");
 
+To hot-remove a disk, call L</guestfs_remove_drive>.  You can call
+this before or after L</guestfs_launch>.  You can only remove disks
+that were previously added with a label.
+
 Backends that support hotplugging do not require that you add
 E<ge> 1 disk before calling launch.  When hotplugging is supported
 you don't need to add any disks.
diff --git a/src/launch-libvirt.c b/src/launch-libvirt.c
index 3eab5676..5b7897d7 100644
--- a/src/launch-libvirt.c
+++ b/src/launch-libvirt.c
@@ -1410,6 +1410,40 @@ hot_add_drive_libvirt (guestfs_h *g, struct drive *drv, size_t drv_index)
   return -1;
 }
 
+/* Hot-remove a drive.  Note the appliance is up when this is called. */
+static int
+hot_remove_drive_libvirt (guestfs_h *g, struct drive *drv, size_t drv_index)
+{
+  virConnectPtr conn = g->virt.connv;
+  virDomainPtr dom = g->virt.domv;
+  xmlChar *xml = NULL;
+
+  if (!conn || !dom) {
+    /* This is essentially an internal error if it happens. */
+    error (g, "%s: conn == NULL or dom == NULL", __func__);
+    return -1;
+  }
+
+  /* Re-create the XML for the disk. */
+  xml = construct_libvirt_xml_hot_add_disk (g, drv, drv_index);
+  if (xml == NULL)
+    return -1;
+
+  /* Detach it. */
+  if (virDomainDetachDeviceFlags (dom, (char *) xml,
+                                  VIR_DOMAIN_DEVICE_MODIFY_LIVE) == -1) {
+    libvirt_error (g, _("could not detach disk from libvirt domain"));
+    goto error;
+  }
+
+  free (xml);
+  return 0;
+
+ error:
+  free (xml);
+  return -1;
+}
+
 static xmlChar *
 construct_libvirt_xml_hot_add_disk (guestfs_h *g, struct drive *drv,
                                     size_t drv_index)
@@ -1449,6 +1483,7 @@ struct attach_ops attach_ops_libvirt = {
   .shutdown = shutdown_libvirt,
   .max_disks = max_disks_libvirt,
   .hot_add_drive = hot_add_drive_libvirt,
+  .hot_remove_drive = hot_remove_drive_libvirt,
 };
 
 #else /* no libvirt or libxml2 at compile time */
diff --git a/src/launch.c b/src/launch.c
index a4e10fef..cc45a10b 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -452,6 +452,60 @@ guestfs__add_cdrom (guestfs_h *g, const char *filename)
   return guestfs__config (g, "-cdrom", filename);
 }
 
+/* Depending on whether we are hotplugging or not, this function
+ * does slightly different things: If not hotplugging, then the
+ * drive just disappears as if it had never been added.  The later
+ * drives "move up" to fill the space.  When hotplugging we have to
+ * do some complex stuff, and we usually end up leaving an empty
+ * (NULL) slot in the g->drives vector.
+ */
+int
+guestfs__remove_drive (guestfs_h *g, const char *label)
+{
+  size_t i;
+  struct drive *drv;
+
+  ITER_DRIVES (g, i, drv) {
+    if (drv->disk_label && STREQ (label, drv->disk_label))
+      goto found;
+  }
+  error (g, _("disk with label '%s' not found"), label);
+  return -1;
+
+ found:
+  if (g->state == CONFIG) {     /* Not hotplugging. */
+    free_drive_struct (drv);
+
+    g->nr_drives--;
+    for (; i < g->nr_drives; ++i)
+      g->drives[i] = g->drives[i+1];
+
+    return 0;
+  }
+  else {                        /* Hotplugging. */
+    if (!g->attach_ops || !g->attach_ops->hot_remove_drive) {
+      error (g, _("the current attach-method does not support hotplugging drives"));
+      return -1;
+    }
+
+    if (guestfs_internal_hot_remove_drive_precheck (g, label) == -1)
+      return -1;
+
+    if (g->attach_ops->hot_remove_drive (g, drv, i) == -1)
+      return -1;
+
+    free_drive_struct (drv);
+    g->drives[i] = NULL;
+    if (i == g->nr_drives-1)
+      g->nr_drives--;
+
+    if (guestfs_internal_hot_remove_drive (g, label) == -1)
+      return -1;
+
+    return 0;
+  }
+}
+
 int
 guestfs__config (guestfs_h *g,
                  const char *qemu_param, const char *qemu_value)
diff --git a/tests/hotplug/Makefile.am b/tests/hotplug/Makefile.am
index 6644930a..9187c766 100644
--- a/tests/hotplug/Makefile.am
+++ b/tests/hotplug/Makefile.am
@@ -18,9 +18,11 @@
 include $(top_srcdir)/subdir-rules.mk
 
 TESTS = \
-	test-hot-add.pl
+	test-hot-add.pl \
+	test-hot-remove.pl
 
 TESTS_ENVIRONMENT = $(top_builddir)/run --test
 
 EXTRA_DIST = \
-	$(TESTS)
+	$(TESTS) \
+	test-hotplug-repeated.pl
diff --git a/tests/hotplug/test-hot-remove.pl b/tests/hotplug/test-hot-remove.pl
new file mode 100755
index 00000000..af30869e
--- /dev/null
+++ b/tests/hotplug/test-hot-remove.pl
@@ -0,0 +1,88 @@
+#!/usr/bin/perl
+# Copyright (C) 2012 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test hot-adding and -removing disks.
+
+use strict;
+use warnings;
+
+use Sys::Guestfs;
+
+my $g = Sys::Guestfs->new ();
+
+# Skip the test if the default attach-method isn't libvirt, since only
+# the libvirt backend supports hotplugging.
+my $attach_method = $g->get_attach_method ();
+unless ($attach_method eq "libvirt" || $attach_method =~ /^libvirt:/) {
+    print "$0: test skipped because attach-method ($attach_method) is not libvirt\n";
+    exit 77
+}
+
+# Create some temporary disks.
+open FILE, ">test1.img" or die "test1.img: $!";
+truncate FILE, 512 * 1024 * 1024 or die "test1.img: truncate: $!";
+close FILE;
+
+open FILE, ">test2.img" or die "test2.img: $!";
+truncate FILE, 512 * 1024 * 1024 or die "test2.img: truncate: $!";
+close FILE;
+
+die unless system ("qemu-img create -f qcow2 test3.img 1G") == 0;
+
+# Hot-add them.  Labels are required.
+$g->add_drive ("test1.img", label => "a"); # autodetect format
+$g->add_drive ("test2.img", label => "b", format => "raw", readonly => 1);
+$g->add_drive ("test3.img", label => "c", format => "qcow2");
+
+# Remove them (before launch).
+$g->remove_drive ("a");
+$g->remove_drive ("b");
+$g->remove_drive ("c");
+
+$g->launch ();
+
+# There should be no drives yet.
+my @devices = $g->list_devices ();
+die unless 0 == @devices;
+
+# Add them again (after launch).
+$g->add_drive ("test1.img", label => "a"); # autodetect format
+$g->add_drive ("test2.img", label => "b", format => "raw", readonly => 1);
+$g->add_drive ("test3.img", label => "c", format => "qcow2");
+
+# Check we can use the disks immediately.
+$g->part_disk ("/dev/disk/guestfs/a", "mbr");
+$g->mkfs ("ext2", "/dev/disk/guestfs/c");
+$g->mkfs ("ext2", "/dev/disk/guestfs/a1");
+
+# Remove them (hotplug this time).
+$g->remove_drive ("a");
+$g->remove_drive ("b");
+$g->remove_drive ("c");
+
+# There should be no drives remaining.
+@devices = $g->list_devices ();
+die unless 0 == @devices;
+
+$g->shutdown ();
+$g->close ();
+
+unlink "test1.img";
+unlink "test2.img";
+unlink "test3.img";
+
+exit 0
diff --git a/tests/hotplug/test-hotplug-repeated.pl b/tests/hotplug/test-hotplug-repeated.pl
new file mode 100755
index 00000000..8c4fc446
--- /dev/null
+++ b/tests/hotplug/test-hotplug-repeated.pl
@@ -0,0 +1,57 @@
+#!/usr/bin/perl
+# Copyright (C) 2012 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program 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 General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Test repeatedly hotplugging a single disk.
+
+use strict;
+use warnings;
+
+use Sys::Guestfs;
+
+my $g = Sys::Guestfs->new ();
+
+# Skip the test if the default attach-method isn't libvirt, since only
+# the libvirt backend supports hotplugging.
+my $attach_method = $g->get_attach_method ();
+unless ($attach_method eq "libvirt" || $attach_method =~ /^libvirt:/) {
+    print "$0: test skipped because attach-method ($attach_method) is not libvirt\n";
+    exit 77
+}
+
+$g->launch ();
+
+# Create a temporary disk.
+open FILE, ">test1.img" or die "test1.img: $!";
+truncate FILE, 512 * 1024 * 1024 or die "test1.img: truncate: $!";
+close FILE;
+
+my $start_t = time ();
+while (time () - $start_t <= 60) {
+    $g->add_drive ("test1.img", label => "a", format => "raw");
+    $g->remove_drive ("a");
+}
+
+# There should be no drives remaining.
+my @devices = $g->list_devices ();
+die unless 0 == @devices;
+
+$g->shutdown ();
+$g->close ();
+
+unlink "test1.img";
+
+exit 0
-- 
cgit