mock patch proposal: unmount everything mounted under chroot

[Mike McLean]

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.

> +        # 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]
> +            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.

> +               cmd = "umount -n %s" % mountpoint
> +               self.root_log.warning("Forcibly unmounting '%s' from chroot." %
> +                       mountpoint)
> +               mock.util.do(cmd, raiseExc=0, shell=True)
>
>      decorate(traceLog())
>      def _yum(self, cmd, returnOutput=0):