On 03/15/2010 09:29 AM, Alan Franzoni wrote:
Hello,
after getting some errors about unmount in mock, most probably caused
by binfmt_misc issues - somewhere some package I was building did
indeed trigger some "automagic" binfmt_misc mount under /proc/sys, I
decided to patch mock to just unmount *all* directories that have been
mounted under such chroot, no matter who mounted them or how they got
mounted.
While I am a fan of extra-paranoid unmounting (as is anyone who has even
run mock clean with an extra mount accidentally left in place), I think
the implementation needs to be more robust.
To go a step further than your suggestion, I would like to see mock's
util.rmtree altered to not cross filesystem boundaries.
Comments on the patch itself inline.
--- /tmp/backend.py.orig 2010-03-15 12:10:30.000000000 +0100
+++ /usr/lib/python2.6/site-packages/mock/backend.py 2010-03-15 14:16:38.000000000 +0100
When submitting a patch, it is best to make sure the patch is can be
easily applied by the developers. Your patch references absolute
pathnames which makes applying a hassle. Depending on invocation, patch
might try to apply the change to the system's backend.py instead of the
development copy, or simply fail to find a file to patch.
@@ -85,14 +85,14 @@
self.pluginConf[key]['cache_topdir'] = self.cache_topdir
self.pluginConf[key]['cachedir'] = self.cachedir
self.pluginConf[key]['root'] = self.sharedRootName
-
- # mount/umount
- self.umountCmds = ['umount -n %s' %
self.makeChrootPath('proc'),
- 'umount -n %s' % self.makeChrootPath('sys')
- ]
+
^^^^ trailing whitespace
+ # mount
self.mountCmds = ['mount -n -t proc mock_chroot_proc %s' %
self.makeChrootPath('proc'),
'mount -n -t sysfs mock_chroot_sysfs %s' %
self.makeChrootPath('sys'),
]
+ # retain umountCmds for interfacing sake, but they're not required
+ # anymore
+ self.umountCmds = []
self.build_log_fmt_str = config['build_log_fmt_str']
self.root_log_fmt_str = config['root_log_fmt_str']
@@ -340,13 +340,6 @@
os.umask(prevMask)
- # mount/umount
- for devUnmtCmd in (
- 'umount -n %s' % self.makeChrootPath('/dev/pts'),
- 'umount -n %s' % self.makeChrootPath('/dev/shm') ):
- if devUnmtCmd not in self.umountCmds:
- self.umountCmds.append(devUnmtCmd)
-
mountopt = 'gid=%d,mode=0620,ptmxmode=0666' %
grp.getgrnam('tty').gr_gid
if kver >= '2.6.29':
mountopt += ',newinstance'
@@ -587,9 +580,19 @@
decorate(traceLog())
def _umountall(self):
"""umount all mounted chroot fs."""
- for cmd in self.umountCmds:
- self.root_log.debug(cmd)
- mock.util.do(cmd, raiseExc=0, shell=True)
+ mountpoints = mock.util.do("cat /proc/mounts", raiseExc=0, shell=True,
returnOutput=1)
You're reading /proc/mounts by running cat in a shell, which is a bit
silly when python can just read the file itself.
+ # umount in reverse mount order to prevent nested mount
issues that
+ # may prevent clean unmount.
+ for mountline in reversed(mountpoints.split("\n")):
+ try:
+ mountpoint = mountline.split(" ")[1]
+ if self.makeChrootPath("/") in mountpoint:
^^
The test "self.makeChrootPath("/") in mountpoint" is not quite
correct.
It could generate false positives (granted this is unlikely), but more
importantly, it could fail to detect a chroot mount when there are
multiple ways to specify it (e.g. with symlinks in use). A more careful
approach would:
- use os.path.realpath() to canonicalize paths before comparing
- retain mount data as a fallback
+ cmd = "umount -n %s" % mountpoint
+ self.root_log.debug(cmd)
+ mock.util.do(cmd, raiseExc=0, shell=True)
+ except:
+ # dont mind about wrong parsing, etc. just go on with the next
+ continue
What are you trying to catch here? You passed raiseExc=0, so the command
is not going to generate an error. What's left? Parsing the line from
/proc/mounts? It looks like the only way that can fail is if
/proc/mounts contains a malformed line. I'm not sure if that can even
happen (and if it does I think it is worth noting instead of ignoring).
Also, unqualified except clauses should be used sparingly, if at all.
decorate(traceLog())
def _yum(self, cmd, returnOutput=0):