mock patch proposal: unmount everything mounted under chroot

[Clark Williams]

On Tue, 16 Mar 2010 12:03:56 -0400
Mike McLean <mikem at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
Url : http://lists.fedoraproject.org/pipermail/buildsys/attachments/20100316/797f0f28/attachment.bin