On Tue, 16 Mar 2010 12:03:56 -0400
Mike McLean <mikem(a)redhat.com> wrote:
On 03/16/2010 10:14 AM, Alan Franzoni wrote:
> OK,
> I attached a reviewed version of the patch to ticket #4 on fedorahosted:
>
>
https://fedorahosted.org/mock/ticket/4
>
> let me know if this seems better.
Much better, thank you.
> diff --git a/py/mock/backend.py b/py/mock/backend.py
> index 7ca6a1d..2eb3bba 100644
> --- a/py/mock/backend.py
> +++ b/py/mock/backend.py
> @@ -587,9 +587,24 @@ class Root(object):
> decorate(traceLog())
> def _umountall(self):
> """umount all mounted chroot fs."""
> + # first try removing all expected mountpoints.
> for cmd in self.umountCmds:
> self.root_log.debug(cmd)
> - mock.util.do(cmd, raiseExc=0, shell=True)
> + try:
> + mock.util.do(cmd, raiseExc=1, shell=True)
> + except mock.exception.Error, e:
> + self.root_log.warning("'%s' failed." % cmd)
It might be better to log the exception string rather than this less
informative warning.
Agreed. Maybe:
self.root_log.warning("'%s': %s" % (cmd, e))
> + # then remove anything that might be left around.
> + mountpoints =
open("/proc/mounts").read().strip().split("\n")
Why the strip? Why not just use open("/proc/mounts").readlines()?
> + # umount in reverse mount order to prevent nested mount issues that
> + # may prevent clean unmount.
> + for mountline in reversed(mountpoints):
> + mountpoint = mountline.split(" ")[1]
I'd probably change the split(" ") to be split(), just to pick up any
runs of whitespace.
> + if self.makeChrootPath("/") in
os.path.realpath(mountpoint):
To be safe, you need to apply realpath to both. There is no guarantee
that makeChrootPath will return an canonical path.
I'm pretty sure that makeChrootPath() was designed to do exactly that
(return a canonical path for the chroot + element). If you know
of a case where it doesn't, that's a bug and we need to fix it.
Clark