[buildd-tools-devel] Bug#691376: Bug#691376: schroot: session recovery mounts $device, not $mount-device, for LVM snapshots

[Roger Leigh]

tags 1.6.3-1 + patch pending
thanks

On Wed, Oct 24, 2012 at 10:23:27PM +0100, Roger Leigh wrote:

Patch to address this issue.  Note that this is not yet completely
tested for all the other block-device-using chroot types.  **Do
not apply until tested.**

commit 19293f7a7b0943997397ef30465cd11686990a9b
Author: Roger Leigh <rleigh at debian.org>
Date:   Wed Oct 24 23:25:10 2012 +0100

    sbuild: Don't call set_mount_device indirectly from ctor
    
    See #691376.
    
    sbuild::chroot_block_device_base::set_device was called from
    the chroot_block_device_base ctor, and called
    sbuild::chroot_facet_mountable::set_mount_device for all chroot
    types except lvm-snapshot.  However, dynamic_cast<> fails during
    construction, leading to the mount device being overwritten, and
    --recover-session mounting the source LV rather than the snapshot
    LV.  This patch moves the call to set_mount_device to the session
    clone operation in sbuild::chroot_facet_session_clonable, which
    is where the equivalent operation is done for lvm-snapshot.  This
    is done prior to lvm-snapshot to ensure that the snapshot is not
    overwritten here either.

diff --git a/sbuild/sbuild-chroot-block-device-base.cc b/sbuild/sbuild-chroot-block-device-base.cc
index 2f2f958..86f3af6 100644
--- a/sbuild/sbuild-chroot-block-device-base.cc
+++ b/sbuild/sbuild-chroot-block-device-base.cc
@@ -46,12 +46,8 @@ chroot_block_device_base::chroot_block_device_base ():
 chroot_block_device_base::chroot_block_device_base
 (const chroot_block_device_base& rhs):
   chroot(rhs),
-  device()
+  device(rhs.device)
 {
-  /// @todo Required to set mount_device.  Remove once no longer
-  /// needed.
-  if (!rhs.device.empty())
-    set_device(rhs.device);
 }
 
 chroot_block_device_base::~chroot_block_device_base ()
@@ -71,16 +67,6 @@ chroot_block_device_base::set_device (std::string const& device)
     throw error(device, DEVICE_ABS);
 
   this->device = device;
-
-  /// @todo: This may not be appropriate for derived classes such as
-  /// lvm_snapshot, since re-setting the device could overwrite the
-  /// mount device.
-  chroot_facet_mountable::ptr pmnt
-    (get_facet<chroot_facet_mountable>());
-#ifdef SBUILD_FEATURE_LVMSNAP
-  if (!dynamic_cast<chroot_lvm_snapshot *>(this))
-#endif
-    pmnt->set_mount_device(this->device);
 }
 
 std::string
diff --git a/sbuild/sbuild-chroot-facet-session-clonable.cc b/sbuild/sbuild-chroot-facet-session-clonable.cc
index 408e9ab..1d924ad 100644
--- a/sbuild/sbuild-chroot-facet-session-clonable.cc
+++ b/sbuild/sbuild-chroot-facet-session-clonable.cc
@@ -19,6 +19,7 @@
 #include <config.h>
 
 #include "sbuild-chroot.h"
+#include "sbuild-chroot-facet-mountable.h"
 #include "sbuild-chroot-facet-session.h"
 #include "sbuild-chroot-facet-session-clonable.h"
 #include "sbuild-chroot-facet-source-clonable.h"
@@ -138,6 +139,19 @@ chroot_facet_session_clonable::clone_session_setup (chroot::ptr&       clone,
     << format("Mount Location: %1%") % clone->get_mount_location()
     << endl;
 
+  /* Block devices need the mount device name specifying. */
+  /* Note that this will be overridden by LVM snapshot, below, so the
+     order here is important. */
+  std::shared_ptr<chroot_block_device_base> blockdevbase(std::dynamic_pointer_cast<chroot_block_device_base>(clone));
+  if (blockdevbase)
+    {
+      chroot_facet_mountable::ptr pmnt
+	(clone->get_facet<chroot_facet_mountable>());
+      if (pmnt)
+	pmnt->set_mount_device(blockdevbase->get_device());
+    }
+
+
 #ifdef SBUILD_FEATURE_LVMSNAP
   /* LVM devices need the snapshot device name specifying. */
   std::shared_ptr<chroot_lvm_snapshot> snapshot(std::dynamic_pointer_cast<chroot_lvm_snapshot>(clone));


-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux    http://people.debian.org/~rleigh/
 `. `'   schroot and sbuild  http://alioth.debian.org/projects/buildd-tools
   `-    GPG Public Key      F33D 281D 470A B443 6756 147C 07B3 C8BC 4083 E800