On 03/15/2010 09:29 AM, Alan Franzoni wrote:
> https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora%20Hosted%20Projects&c omponent=mock
>
> but even though I registered I get this message:
>
> Sorry, entering a bug into the product Fedora Hosted Projects has been disabled.

Odd, though I suppose Fedora Hosted projects ought to use Trac.
https://fedorahosted.org/mock/newticket

Or you could just file against mock in Fedora itself.
https://bugzilla.redhat.com/enter_bug.cgi?product=Fedora&component=mock

--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

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("
")):
> + 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):
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

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.
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

On 3/15/10 5:23 PM, Mike McLean wrote:
[cut]

Just one more thing, since I seem to have messed up with my mail
accounts yesterday, hence my previous answer wasn't delivered to the
list; I'd like to apologize about my previous patch, it was just a
quick-and-dirty hack I submitted for initial review of my approach.

> Odd, though I suppose Fedora Hosted projects ought to use Trac.
> https://fedorahosted.org/mock/newticket

That's fine, just beware that it's the fedoraproject page for mock
(http://fedoraproject.org/wiki/Projects/Mock) that reccomends using
bugzilla for tickets :-)


--
Alan Franzoni
contact me at public@[mysurname].eu
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

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("
")

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):

--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

On Tue, 16 Mar 2010 12:03:56 -0400
Mike McLean <mikem@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("
")
>
> 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
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

On 03/16/2010 02:50 PM, Clark Williams wrote:
> 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.

It doesn't expand symlinks or even fully normalize. For the rest of mock
that is fine, but when you need to compare paths coming from different
sources you want something stronger. The kernel is not going to use
makeChrootPath to populate /proc/mounts

Actually, I realized we probably to use os.path.samefile() to make the
comparison. This would eliminate the need for realpath. Otoh, it does
raise an exception if either fails to stat so we'll probably want to
catch that (granted, such a situation shouldn't really happen).
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

On 03/16/2010 02:50 PM, Clark Williams wrote:
>>> + 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))

All the exception strings from do() already include the command text. I
was thinking simply:
self.root_log.warning(str(e))
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

On 3/16/10 8:28 PM, Mike McLean wrote:
[cut]

I updated the ticket and the patch:

https://fedorahosted.org/mock/ticket/r

I think I addressed most of your concerns, the only thing I could not
sort out was how would you use samefile() . I just used realpath() for
both checks.

Bye!

--
Alan Franzoni
contact me at public@[mysurname].eu
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys

I apologize for any duplicate message you might receive, my smtp server
had some problems lately.

On 3/16/10 8:28 PM, Mike McLean wrote:
[cut]

I updated the ticket and the patch:

https://fedorahosted.org/mock/ticket/r

I think I addressed most of your concerns, the only thing I could not
sort out was how would you use samefile() . I just used realpath() for
both checks.

Bye!

--
Alan Franzoni
contact me at public@[mysurname].eu
--
buildsys mailing list
buildsys@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/buildsys