On Tue, 16 Mar 2010 12:03:56 -0400 Mike McLean <[email protected]> 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
signature.asc
Description: PGP signature
-- buildsys mailing list [email protected] https://admin.fedoraproject.org/mailman/listinfo/buildsys
