Andreas Bombe <[EMAIL PROTECTED]> writes:

> On Wed, May 31, 2006 at 10:53:18AM +0100, Roger Leigh wrote:
>> Andreas Bombe <[EMAIL PROTECTED]> writes:
>> 
>> > The session cleanup in 10mount ignores failures of umount invocations
>> > and cleanup continues.  In the case of file chroots with a /home bind
>> > mount that failed to umount, the rm -rf in 05file blindly descends into
>> > the system /home with obvious unpretty results.

>> There are a few possibilities here.
>> 
>> 1) 10mount should exit with an error if umount fails.
>> 
>>    Caveat: if the session is ended with the setup scripts having
>>    failed, this would require manual cleanup by the system admin.
>>    This needs additional work in session::setup_chroot() in
>>    sbuild-session.cc, so that the session is not ended if the scripts
>>    fail.  This means not removing the session file from
>>    /var/lib/schroot/session/ on failure.
>> 
>>    Currently, because of the above consideration, the "setup-stop"
>>    phase of the session scripts can not fail.
>
> If a umount fails it will require manual admin intervention anyway so
> that wouldn't make much of a difference.  Making the rm -rf safe is
> still required in any case, I'd say.

OK, I agree here.  In the patch below, any umount failure will abort
the cleanup.

>> 2) 05file must check if any filesystems are mounted under the chroot
>>    root before running rm -rf.  Is there a portable and reliable way
>>    of doing this?  Would
>> 
>>      if mount | grep "$CHROOT_MOUNT_LOCATION"; then
>>        :
>>      else
>>        rm -rf "$CHROOT_MOUNT_LOCATION" || true
>>      fi
>> 
>>    be sufficient?
>
> I don't think that is safe.  It depends on all mounts being recorded in
> /etc/mtab, which is not the case if something *inside* the chroot
> mounted something, for example.

> I thought about rm with a "do not cross filesystems" option, still that
> wouldn't help because binds may well be on the same filesystem.  There
> are no usable criteria for using "find ... -exec rm ..." either.

To work around this, I wrote a program called schroot-listmounts,
which uses /proc/mounts to get a list:

./schroot/schroot-listmounts -m /srv/chroot/sid
/srv/chroot/sid/tmp
/srv/chroot/sid/home
/srv/chroot/sid/dev/shm
/srv/chroot/sid/dev/pts
/srv/chroot/sid

The shell script can read this list and unmount them in turn.

> The only way I know to be sure there are no submounts is to mount --bind
> the chroot to a temporary location and rm -rf there, then unmount the
> temporary bind and rmdir the chroot.
>
> The rmdir will fail safely if the chroot isn't empty then. Even before,
> the rm -rf of the temp bind will fail safely upon trying to remove an
> empty directory used as a mount point in the chroot.

I think I now have this solved.  If the chroot directories are
unmounted, schroot-listmounts will list nothing:

./schroot/schroot-listmounts -m 
/var/lib/schroot/mount/testsnap-c4e53f96-e507-40e8-8cf4-fcfd31c17c32
[no output]

so in this case we can be sure it's safe to "rm -rf".  If there's any
output at all, we know it's not safe.


I've attached a patch for this.  Note: it's not yet tested; this is
just to give you an idea of what I'm thinking of as a solution.  If
the testing works out, and you are happy with it, I'll include this in
the next release.

 ChangeLog                             |   43 ++++++++
 NEWS                                  |   10 +
 debian/changelog                      |    3
 schroot/Makefile.am                   |   10 +
 schroot/sbuild-chroot-block-device.cc |    3
 schroot/sbuild-chroot-block-device.h  |    9 -
 schroot/sbuild-chroot-file.cc         |    5
 schroot/sbuild-chroot-file.h          |    9 -
 schroot/sbuild-chroot-lvm-snapshot.cc |    5
 schroot/sbuild-chroot-lvm-snapshot.h  |    9 -
 schroot/sbuild-chroot-plain.cc        |    5
 schroot/sbuild-chroot-plain.h         |    9 -
 schroot/sbuild-chroot.cc              |   13 ++
 schroot/sbuild-chroot.h               |   42 +++++++
 schroot/sbuild-session.cc             |    6 -
 schroot/sbuild-util.cc                |   13 ++
 schroot/sbuild-util.h                 |   12 ++
 schroot/schroot-listmounts-options.cc |   83 +++++++++++++++
 schroot/schroot-listmounts-options.h  |   59 +++++++++++
 schroot/schroot-listmounts.cc         |  181 ++++++++++++++++++++++++++++++++++
 schroot/setup/05file                  |   15 ++
 schroot/setup/10mount                 |   31 +----
 test/sbuild-chroot.cc                 |    3
 23 files changed, 522 insertions(+), 56 deletions(-)


Regards,
Roger

-- 
Roger Leigh
                Printing on GNU/Linux?  http://gutenprint.sourceforge.net/
                Debian GNU/Linux        http://www.debian.org/
                GPG Public Key: 0x25BFB848.  Please sign and encrypt your mail.
Index: test/sbuild-chroot.cc
===================================================================
--- test/sbuild-chroot.cc	(revision 697)
+++ test/sbuild-chroot.cc	(working copy)
@@ -53,7 +53,8 @@
 
   virtual void
   setup_lock (setup_type type,
-	      bool       lock)
+	      bool       lock,
+	      int        status)
   {}
 
   virtual sbuild::chroot::session_flags

Property changes on: schroot
___________________________________________________________________
Name: svn:ignore
   - .deps
.libs
Makefile
Makefile.in
*.la
*.lo
*.gch
stamp-h2
sbuild-config.h
dchroot
schroot
schroot-releaselock
dchroot.1
schroot.1
schroot-setup.5
schroot.conf.5

   + .deps
.libs
Makefile
Makefile.in
*.la
*.lo
*.gch
stamp-h2
sbuild-config.h
dchroot
schroot
schroot-listmounts
schroot-releaselock
dchroot.1
schroot.1
schroot-setup.5
schroot.conf.5


Index: schroot/sbuild-session.cc
===================================================================
--- schroot/sbuild-session.cc	(revision 697)
+++ schroot/sbuild-session.cc	(working copy)
@@ -472,7 +472,7 @@
 
   try
     {
-      session_chroot->setup_lock(setup_type, true);
+      session_chroot->lock(setup_type);
     }
   catch (chroot::error const& e)
     {
@@ -480,7 +480,7 @@
       try
 	{
 	  // Release lock, which also removes session metadata.
-	  session_chroot->setup_lock(setup_type, false);
+	  session_chroot->unlock(setup_type, 0);
 	}
       catch (chroot::error const& ignore)
 	{
@@ -602,7 +602,7 @@
 
   try
     {
-      session_chroot->setup_lock(setup_type, false);
+      session_chroot->unlock(setup_type, exit_status);
     }
   catch (chroot::error const& e)
     {
Index: schroot/sbuild-chroot-lvm-snapshot.cc
===================================================================
--- schroot/sbuild-chroot-lvm-snapshot.cc	(revision 697)
+++ schroot/sbuild-chroot-lvm-snapshot.cc	(working copy)
@@ -116,7 +116,8 @@
 
 void
 chroot_lvm_snapshot::setup_lock (setup_type type,
-				 bool       lock)
+				 bool       lock,
+				 int        status)
 {
   std::string device;
   struct stat statbuf;
@@ -187,7 +188,7 @@
 
   /* Create or unlink session information. */
   if ((type == SETUP_START && lock == true) ||
-      (type == SETUP_STOP && lock == false))
+      (type == SETUP_STOP && lock == false && status == 0))
     {
       bool start = (type == SETUP_START);
       setup_session_info(start);
Index: schroot/sbuild-chroot-lvm-snapshot.h
===================================================================
--- schroot/sbuild-chroot-lvm-snapshot.h	(revision 697)
+++ schroot/sbuild-chroot-lvm-snapshot.h	(working copy)
@@ -94,15 +94,16 @@
     virtual void
     setup_env (environment& env);
 
-    virtual void
-    setup_lock (setup_type type,
-		bool       lock);
-
     virtual session_flags
     get_session_flags () const;
 
   protected:
     virtual void
+    setup_lock (setup_type type,
+		bool       lock,
+		int        status);
+
+    virtual void
     print_details (std::ostream& stream) const;
 
     virtual void
Index: schroot/sbuild-util.h
===================================================================
--- schroot/sbuild-util.h	(revision 697)
+++ schroot/sbuild-util.h	(working copy)
@@ -52,6 +52,18 @@
 	   char        separator = '/');
 
   /**
+   * Normalise a pathname.  This strips all trailing separators, and
+   * duplicate separators within a path.
+   *
+   * @param name the path to normalise.
+   * @param separator the separation delimiting directories.
+   * @returns the normalised name.
+   */
+  std::string
+  normalname (std::string name,
+	      char        separator = '/');
+
+  /**
    * Convert a string_list into a string.  The strings are
    * concatenated using separator as a delimiter.
    *
Index: schroot/setup/10mount
===================================================================
--- schroot/setup/10mount	(revision 697)
+++ schroot/setup/10mount	(working copy)
@@ -23,24 +23,17 @@
     mount $VERBOSE $1 "$2" "$3"
 }
 
-# Unmount a filesystem
-# $1: mount location
-do_umount()
-{
-    if [ "$AUTH_VERBOSITY" = "verbose" ]; then
-	echo "Unmounting $1"
-    fi
-    if [ -d "$1" ]; then
-	umount "$1" || true
-    fi
-}
-
 # Unmount all filesystem under specified location
 # $1: mount base location
 do_umount_all()
 {
-    for dir in $(mount | grep $1 | sed -e 's/.* on \(.*\) type.*/\1/' | sort -r); do
-	do_umount "$dir"
+    "$LIBEXEC_DIR/schroot-listmounts" -m "$1" |
+    while read mountloc; do
+	if [ "$AUTH_VERBOSITY" = "verbose" ]; then
+	    echo "Unmounting $mountloc"
+	fi
+	umount "$mountloc"
+	fi
     done
 }
 
@@ -91,17 +84,11 @@
 
     elif [ $1 = "setup-stop" ]; then
 
-	do_umount "${CHROOT_PATH}/tmp"
-	do_umount "${CHROOT_PATH}/home"
-	do_umount "${CHROOT_PATH}/dev/shm"
-	do_umount "${CHROOT_PATH}/dev/pts"
-	do_umount "${CHROOT_PATH}/proc"
+	do_umount_all "$CHROOT_MOUNT_LOCATION"
 
 	if [ "$CHROOT_TYPE" != "file" ]; then
-	    do_umount "$CHROOT_MOUNT_LOCATION"
-
 	    if echo "$CHROOT_MOUNT_LOCATION" | grep -q "^$MOUNT_DIR/"; then
-		rmdir "$CHROOT_MOUNT_LOCATION" || true
+		rmdir "$CHROOT_MOUNT_LOCATION"
 	    fi
 	fi
 
Index: schroot/setup/05file
===================================================================
--- schroot/setup/05file	(revision 697)
+++ schroot/setup/05file	(working copy)
@@ -88,8 +88,21 @@
 	    cd "$CHROOT_MOUNT_LOCATION" && repack_file
 	fi
 
-	rm -rf "$CHROOT_MOUNT_LOCATION" || true
+	"$LIBEXEC_DIR/schroot-listmounts" -m "$CHROOT_MOUNT_LOCATION" |
+	if read mountloc; then
+	    if [ "$AUTH_VERBOSITY" = "verbose" ]; then
+		echo "Not purging $CHROOT_MOUNT_LOCATION; filesystems are mounted:"
+		"$LIBEXEC_DIR/schroot-listmounts" -m "$CHROOT_MOUNT_LOCATION"
+	    fi
+	    exit 1
+	fi
 
+	if [ "$AUTH_VERBOSITY" = "verbose" ]; then
+	    echo "Purging $CHROOT_MOUNT_LOCATION"
+	fi
+
+	rm -rf "$CHROOT_MOUNT_LOCATION"
+
     fi
 
 fi
Index: schroot/sbuild-chroot-plain.cc
===================================================================
--- schroot/sbuild-chroot-plain.cc	(revision 697)
+++ schroot/sbuild-chroot-plain.cc	(working copy)
@@ -85,14 +85,15 @@
 
 void
 chroot_plain::setup_lock (setup_type type,
-			  bool       lock)
+			  bool       lock,
+			  int        status)
 {
   /* By default, plain chroots do no locking. */
   /* Create or unlink session information. */
   if (get_run_setup_scripts() == true)
     {
       if ((type == SETUP_START && lock == true) ||
-	  (type == SETUP_STOP && lock == false))
+	  (type == SETUP_STOP && lock == false && status == 0))
 	{
 	  bool start = (type == SETUP_START);
 	  setup_session_info(start);
Index: schroot/sbuild-chroot-plain.h
===================================================================
--- schroot/sbuild-chroot-plain.h	(revision 697)
+++ schroot/sbuild-chroot-plain.h	(working copy)
@@ -68,15 +68,16 @@
     virtual void
     setup_env (environment& env);
 
-    virtual void
-    setup_lock (setup_type type,
-		bool       lock);
-
     virtual session_flags
     get_session_flags () const;
 
   protected:
     virtual void
+    setup_lock (setup_type type,
+		bool       lock,
+		int        status);
+
+    virtual void
     print_details (std::ostream& stream) const;
 
     virtual void
Index: schroot/schroot-listmounts-options.h
===================================================================
--- schroot/schroot-listmounts-options.h	(revision 0)
+++ schroot/schroot-listmounts-options.h	(revision 0)
@@ -0,0 +1,59 @@
+/* Copyright © 2005-2006  Roger Leigh <[EMAIL PROTECTED]>
+ *
+ * schroot 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.
+ *
+ * schroot 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., 59 Temple Place, Suite 330, Boston,
+ * MA  02111-1307  USA
+ *
+ *********************************************************************/
+
+#ifndef SCHROOT_LISTMOUNTS_OPTIONS_H
+#define SCHROOT_LISTMOUNTS_OPTIONS_H
+
+#include <string>
+
+namespace schroot_listmounts
+{
+
+  /**
+   * schroot-listmounts command-line options.
+   */
+  class options {
+  public:
+    /**
+     * The constructor.
+     *
+     * @param argc the number of arguments.
+     * @param argv the list of arguments.
+     */
+    options (int   argc,
+	     char *argv[]);
+
+    /// The destructor.
+    virtual ~options ();
+
+    /// The mountpoint to check..
+    std::string mountpoint;
+    /// Display version information.
+    bool        version;
+  };
+
+}
+
+#endif /* SCHROOT_LISTMOUNTS_OPTIONS_H */
+
+/*
+ * Local Variables:
+ * mode:C++
+ * End:
+ */
Index: schroot/sbuild-chroot-block-device.cc
===================================================================
--- schroot/sbuild-chroot-block-device.cc	(revision 697)
+++ schroot/sbuild-chroot-block-device.cc	(working copy)
@@ -111,7 +111,8 @@
 
 void
 chroot_block_device::setup_lock (setup_type type,
-				 bool       lock)
+				 bool       lock,
+				 int        status)
 {
   struct stat statbuf;
 
Index: schroot/sbuild-chroot-block-device.h
===================================================================
--- schroot/sbuild-chroot-block-device.h	(revision 697)
+++ schroot/sbuild-chroot-block-device.h	(working copy)
@@ -106,15 +106,16 @@
     virtual void
     setup_env (environment& env);
 
-    virtual void
-    setup_lock (setup_type type,
-		bool       lock);
-
     virtual session_flags
     get_session_flags () const;
 
   protected:
     virtual void
+    setup_lock (setup_type type,
+		bool       lock,
+		int        status); 
+
+    virtual void
     print_details (std::ostream& stream) const;
 
     virtual void
Index: schroot/sbuild-chroot.cc
===================================================================
--- schroot/sbuild-chroot.cc	(revision 697)
+++ schroot/sbuild-chroot.cc	(working copy)
@@ -316,6 +316,19 @@
 }
 
 void
+sbuild::chroot::lock (setup_type type)
+{
+  setup_lock(type, true, 0);
+}
+
+void
+sbuild::chroot::unlock (setup_type type,
+			int        status)
+{
+  setup_lock(type, false, status);
+}
+
+void
 sbuild::chroot::print_details (std::ostream& stream) const
 {
   if (this->active == true)
Index: schroot/sbuild-chroot-file.cc
===================================================================
--- schroot/sbuild-chroot-file.cc	(revision 697)
+++ schroot/sbuild-chroot-file.cc	(working copy)
@@ -97,7 +97,8 @@
 
 void
 chroot_file::setup_lock (setup_type type,
-			 bool       lock)
+			 bool       lock,
+			 int        status)
 {
   // Check ownership and permissions.
   if (type == SETUP_START && lock == true)
@@ -122,7 +123,7 @@
   /* By default, file chroots do no locking. */
   /* Create or unlink session information. */
   if ((type == SETUP_START && lock == true) ||
-      (type == SETUP_STOP && lock == false))
+      (type == SETUP_STOP && lock == false && status == 0))
     {
 
       bool start = (type == SETUP_START);
Index: schroot/sbuild-chroot-file.h
===================================================================
--- schroot/sbuild-chroot-file.h	(revision 697)
+++ schroot/sbuild-chroot-file.h	(working copy)
@@ -76,15 +76,16 @@
     virtual void
     setup_env (environment& env);
 
-    virtual void
-    setup_lock (setup_type type,
-		bool       lock);
-
     virtual session_flags
     get_session_flags () const;
 
   protected:
     virtual void
+    setup_lock (setup_type type,
+		bool       lock,
+		int        status);
+
+    virtual void
     print_details (std::ostream& stream) const;
 
     virtual void
Index: schroot/sbuild-util.cc
===================================================================
--- schroot/sbuild-util.cc	(revision 697)
+++ schroot/sbuild-util.cc	(working copy)
@@ -109,6 +109,19 @@
 }
 
 std::string
+sbuild::normalname (std::string name,
+		    char        separator)
+{
+  // Remove trailing separators
+  std::string::size_type cur = name.length();
+  while (cur - 1 != 0 && name[cur - 1] == separator)
+    --cur;
+  name.resize(cur);
+
+  return remove_duplicates(name, separator);
+}
+
+std::string
 sbuild::string_list_to_string (sbuild::string_list const& list,
 			       std::string const&         separator)
 {
Index: schroot/schroot-listmounts.cc
===================================================================
--- schroot/schroot-listmounts.cc	(revision 0)
+++ schroot/schroot-listmounts.cc	(revision 0)
@@ -0,0 +1,181 @@
+/* Copyright © 2005-2006  Roger Leigh <[EMAIL PROTECTED]>
+ *
+ * schroot 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.
+ *
+ * schroot 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., 59 Temple Place, Suite 330, Boston,
+ * MA  02111-1307  USA
+ *
+ *********************************************************************/
+
+#include <config.h>
+
+#include <cerrno>
+#include <cstdlib>
+#include <cstdio>
+#include <cstring>
+#include <iostream>
+#include <locale>
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#include <mntent.h>
+
+#include <boost/format.hpp>
+#include <boost/program_options.hpp>
+
+#include <lockdev.h>
+
+#include "sbuild.h"
+
+#include "schroot-listmounts-options.h"
+
+using std::endl;
+using boost::format;
+namespace opt = boost::program_options;
+
+using namespace schroot_listmounts;
+
+/**
+ * Print version information.
+ *
+ * @param stream the stream to output to.
+ */
+void
+print_version (std::ostream& stream)
+{
+  stream << format(_("schroot-listmounts (Debian sbuild) %1%\n")) % VERSION
+	 << _("Written by Roger Leigh\n\n")
+	 << _("Copyright (C) 2004-2006 Roger Leigh\n")
+	 << _("This is free software; see the source for copying conditions.  There is NO\n"
+	      "warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.\n")
+	 << std::flush;
+}
+
+/**
+ * List mounts.
+ *
+ * @param mountfile the file containing the database of mounted filesystems.
+ * @param mountpoint the mount point to check for.
+ */
+sbuild::string_list
+list_mounts (std::string const& mountfile,
+	     std::string const& mountpoint)
+{
+  sbuild::string_list ret;
+
+  std::string to_find = sbuild::normalname(mountpoint);
+
+  std::FILE *mntdb = std::fopen(mountfile.c_str(), "r");
+  if (mntdb == 0)
+    {
+      format fmt(_("%1%: Failed to open: %2%"));
+      fmt % mountfile % std::strerror(errno);
+      throw sbuild::runtime_error(fmt.str());
+    }
+
+  mntent *mount;
+  while ((mount = getmntent(mntdb)) != 0)
+    {
+      std::string mount_dir(mount->mnt_dir);
+      if (to_find == "/" ||
+	  (mount_dir.find(to_find) == 0 &&
+	   (// Names are the same.
+	    mount_dir.size() == to_find.size() ||
+	    // Must have a following /, or not the same directory.
+	    (mount_dir.size() > to_find.size() &&
+	     mount_dir[to_find.size()] == '/'))))
+	ret.push_back(mount_dir);
+    }
+
+  std::cout << std::flush;
+
+  if (std::fclose(mntdb) == EOF)
+    {
+      format fmt(_("%1%: Failed to close: %2%"));
+      fmt % mountfile % std::strerror(errno);
+      throw sbuild::runtime_error(fmt.str());
+    }
+
+  return ret;
+}
+
+/**
+ * Main routine.
+ *
+ * @param argc the number of arguments
+ * @param argv argument vector
+ *
+ * @returns 0 on success, 1 on failure or the exit status of the
+ * chroot command.
+ */
+int
+main (int   argc,
+      char *argv[])
+{
+  try
+    {
+      // Set up locale.
+      std::locale::global(std::locale(""));
+      std::cout.imbue(std::locale());
+      std::cerr.imbue(std::locale());
+
+      bindtextdomain (GETTEXT_PACKAGE, LOCALEDIR);
+      textdomain (GETTEXT_PACKAGE);
+
+#ifndef SBUILD_DEBUG
+      sbuild::debug_level = sbuild::DEBUG_CRITICAL;
+#else
+      sbuild::debug_level = sbuild::DEBUG_NONE;
+#endif
+
+      // Parse command-line options.
+      options opts(argc, argv);
+
+      if (opts.version)
+	{
+	  print_version(std::cerr);
+	  exit(EXIT_SUCCESS);
+	}
+
+      if (opts.mountpoint.empty())
+	{
+	  sbuild::log_error() << _("No mountpoint specified") << endl;
+	  exit (EXIT_FAILURE);
+	}
+
+      // Check mounts.
+      sbuild::string_list mounts =
+	list_mounts("/proc/mounts", opts.mountpoint);
+
+      for (sbuild::string_list::const_reverse_iterator pos = mounts.rbegin();
+	   pos != mounts.rend();
+	   ++pos)
+	std::cout << *pos << '\n';
+      std::cout << std::flush;
+
+      exit (EXIT_SUCCESS);
+    }
+  catch (std::exception const& e)
+    {
+      sbuild::log_error() << e.what() << endl;
+
+      exit(EXIT_FAILURE);
+    }
+}
+
+/*
+ * Local Variables:
+ * mode:C++
+ * End:
+ */
Index: schroot/sbuild-chroot.h
===================================================================
--- schroot/sbuild-chroot.h	(revision 697)
+++ schroot/sbuild-chroot.h	(working copy)
@@ -368,12 +368,27 @@
      * An error will be thrown on failure.
      *
      * @param type the type of setup being performed
-     * @param lock true to lock, false to unlock
      */
-    virtual void
-    setup_lock (setup_type type,
-		bool       lock) = 0;
+    void
+    lock (setup_type type);
 
+    /**
+     * Unlock a chroot during setup.  The locking technique (if any) may
+     * vary depending upon the chroot type and setup stage.  For
+     * example, during creation of an LVM snapshot a block device
+     * might require locking, but afterwards this will change to the
+     * new block device.
+     *
+     * An error will be thrown on failure.
+     *
+     * @param type the type of setup being performed
+     * @param status the exit status of the setup commands (0 for
+     * success, nonzero for failure).
+     */
+    void
+    unlock (setup_type type,
+	    int        status);
+
   protected:
     /**
      * Set up persistent session information.
@@ -383,6 +398,25 @@
     virtual void
     setup_session_info (bool start);
 
+    /**
+     * Unlock a chroot during setup.  The locking technique (if any) may
+     * vary depending upon the chroot type and setup stage.  For
+     * example, during creation of an LVM snapshot a block device
+     * might require locking, but afterwards this will change to the
+     * new block device.
+     *
+     * An error will be thrown on failure.
+     *
+     * @param type the type of setup being performed
+     * @param lock true to lock, false to unlock
+     * @param status the exit status of the setup commands (0 for
+     * success, nonzero for failure).
+     */
+    virtual void
+    setup_lock(setup_type type,
+	       bool       lock,
+	       int        status) = 0;
+
   public:
     /**
      * Get the session flags of the chroot.  These determine how the
Index: schroot/Makefile.am
===================================================================
--- schroot/Makefile.am	(revision 697)
+++ schroot/Makefile.am	(working copy)
@@ -38,7 +38,7 @@
 endif
 
 bin_PROGRAMS = $(dchroot) schroot
-pkglibexec_PROGRAMS = schroot-releaselock
+pkglibexec_PROGRAMS = schroot-releaselock schroot-listmounts
 
 #BUILT_SOURCES = sbuild.gch
 #CLEANFILES = sbuild.gch
@@ -129,6 +129,14 @@
 schroot_releaselock_CFLAGS = $(AM_CFLAGS) $(SCHROOT_CFLAGS)
 schroot_releaselock_LDADD = libsbuild.la
 
+schroot_listmounts_SOURCES =		\
+	schroot-listmounts-options.h	\
+	schroot-listmounts-options.cc	\
+	schroot-listmounts.cc
+schroot_listmounts_CFLAGS = $(AM_CFLAGS) $(SCHROOT_CFLAGS)
+schroot_listmounts_LDADD = libsbuild.la
+
+
 pkgsysconfdir = $(PACKAGE_SYSCONF_DIR)
 pkgsysconf_DATA = schroot.conf
 
Index: schroot/schroot-listmounts-options.cc
===================================================================
--- schroot/schroot-listmounts-options.cc	(revision 0)
+++ schroot/schroot-listmounts-options.cc	(revision 0)
@@ -0,0 +1,83 @@
+/* Copyright © 2005-2006  Roger Leigh <[EMAIL PROTECTED]>
+ *
+ * schroot 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.
+ *
+ * schroot 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., 59 Temple Place, Suite 330, Boston,
+ * MA  02111-1307  USA
+ *
+ *********************************************************************/
+
+#include <config.h>
+
+#include <iostream>
+
+#include <stdlib.h>
+
+#include <boost/format.hpp>
+#include <boost/program_options.hpp>
+
+#include "sbuild.h"
+
+#include "schroot-listmounts-options.h"
+
+using std::endl;
+using boost::format;
+namespace opt = boost::program_options;
+using namespace schroot_listmounts;
+
+options::options (int   argc,
+		  char *argv[]):
+  mountpoint(),
+  version(0)
+{
+  opt::options_description general(_("General options"));
+  general.add_options()
+    ("help,?", _("Show help options"))
+    ("version,V",
+     _("Print version information"));
+
+  opt::options_description lock(_("Lock options"));
+  lock.add_options()
+    ("mountpoint,m", opt::value<std::string>(&this->mountpoint),
+     _("Mountpoint to check (full path)"));
+
+
+  opt::options_description global;
+  global.add(general).add(lock);
+
+  opt::variables_map vm;
+  opt::store(opt::parse_command_line(argc, argv, global), vm);
+  opt::notify(vm);
+
+  if (vm.count("help"))
+    {
+      std::cout
+	<< _("Usage:") << '\n'
+	<< _("  schroot-listmounts [OPTION...] - list mounts") << '\n'
+	<< global << std::flush;
+      exit(EXIT_SUCCESS);
+}
+
+  if (vm.count("version"))
+    this->version = true;
+}
+
+options::~options ()
+{
+}
+
+/*
+ * Local Variables:
+ * mode:C++
+ * End:
+ */
Index: debian/changelog
===================================================================
--- debian/changelog	(revision 697)
+++ debian/changelog	(working copy)
@@ -5,6 +5,9 @@
   * schroot/schroot.1.in, schroot/schroot.conf.5.in: Correct ambiguity and
     mistakes in the documentation (Closes: #369633).  Thanks to Andreas
     Bombe.
+  * 05file and 10mount take additional steps to ensure that filesystems
+    are umounted correctly, and that no chroot will be purged if there are
+    mounted filesystems inside it (Closes: #369626).
 
  --
 
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 697)
+++ ChangeLog	(working copy)
@@ -1,3 +1,46 @@
+2006-06-09  Roger Leigh  <[EMAIL PROTECTED]>
+
+	* debian/changelog: Close #369626.
+
+	* NEWS: Document 05file and 10mount changes.
+
+	* schroot/setup/05file: Use schroot-listmounts to check if there
+	are any mounted filesystems before purging the chroot.
+
+	* schroot/setup/10mount: Use schroot-listmounts to unmount all
+	filesystems in a chroot.  Exit with an error if unmounting fails.
+
+	* test/sbuild-chroot.cc: Implement the new form of
+	sbuild::chroot::setup_lock().
+
+	* schroot/schroot-listmounts-options.(cc|h): New files.  These are
+	the command-line option parser for schroot-listmounts.
+
+	* schroot/schroot-listmounts.cc: New file.  This is a program to
+	list all of the mounts under a given mountpoint.
+
+	* schroot/sbuild-session.cc (setup_chroot): Use the chroot lock
+	and unlock methods, in place of setup_lock.
+
+	* schroot/sbuild-chroot-plain.cc, schroot/sbuild-chroot-file.cc,
+	schroot/sbuild-chroot-block-device.cc,
+	schroot/sbuild-chroot-lvm-snapshot.cc: setup_lock only removes the
+	session (using setup_session_info) if the setup scripts succeeded.
+
+	* schroot/sbuild-chroot-plain.h, schroot/sbuild-chroot-file.h,
+	schroot/sbuild-chroot-block-device.h,
+	schroot/sbuild-chroot-lvm-snapshot.h: setup_lock is protected and
+	has an additional status argument.
+
+	* schroot/sbuild-chroot.cc
+	(lock): New function; calls setup_lock.
+	(unlock): New function; calls setup_lock.
+
+	* schroot/sbuild-chroot.h: Replace setup_lock with lock and unlock
+	methods.  Unlock takes a status argument which indicates if the
+	setup scripts failed or not.  setup_lock is now a protected
+	virtual method used by lock and unlock.
+
 2006-06-08  Roger Leigh  <[EMAIL PROTECTED]>
 
 	* debian/changelog: Close #369633.
Index: NEWS
===================================================================
--- NEWS	(revision 697)
+++ NEWS	(working copy)
@@ -9,8 +9,16 @@
 
 * Major changes in 0.2.11:
 
-  Bugfixes only.
+  1) The 10mount script, used to unmount filesystem in a chroot, will
+     exit with an error if unmounting fails (for safety).  It also
+     uses /proc/mounts (via a new program, schroot-listmounts) to
+     ensure all filesystems in the chroot are unmounted.
 
+  2) The 05file script, used to unpack and repack chroot archives,
+     will use schroot-listmounts to check if any filesystems are
+     mounted before purging the chroot.  This is in order to avoid
+     dataloss.
+
 * Major changes in 0.2.10:
 
   Bugfixes only.

Reply via email to