Re: [gentoo-portage-dev] Plugin backport PATCH (1/2)/(2/2)

[Jason Stubbs]

On Saturday 19 November 2005 08:47, Brian Harring wrote:

> On Sat, Nov 19, 2005 at 12:43:43AM +0900, Jason Stubbs wrote:
> > +++ ./pym/portage_file.py       2005-11-16 01:56:19.157073160 -0500
> >
> > +                       # if the dir perms would lack +wx, we have to force it
> > +                       force_temp_perms = ((mode & 0300) != 0300)
> > +                       resets = []
> > +                       apath = normpath(os.path.abspath(path))
> > +                       sticky_parent = False
> > +                       creating = False
> > +                       
> > +                       for dir in apath.split(os.path.sep):
> > +                               base = os.path.join(base,dir)
> > +                               try:
> > +                                       st = os.stat(base)
> > +                                       # something exists.  why are we doing isdir?
> > +                                       if not stat.S_ISDIR(st.st_mode):
> > +                                               return False
> >
> > What's with the comment here?
> I blame crack.
> Rhetorical question in the code, which isn't very clear in hindsight


Dunno.. The code makes complete sense to me by itself. The comment only

confuses.



> >
> > +                                       # if it's a subdir, we need +wx at least
> > +                                       if apath != base:
> > +                                               if ((st.st_mode & 0300) != 0300):
> > +                                                       try:                    os.chmod(base, (st.st_mode | 0300))
> > +                                                       except OSError: return False
> > +                                                       resets.append((base, st.st_mode))
> > +                                               sticky_parent = (st.st_gid & stat.S_ISGID)
> >
> > <snip>
> >
> > +                       try:
> > +                               for base, m in reversed(resets):
> > +                                       os.chmod(base, m)
> > +                               if uid != -1 or gid != -1:
> > +                                       os.chown(base, uid, gid)
> > +                       except OSError:
> > +                               return False
> >
> > If for some reason the parent dir originally has perms 0000, this code will
> > incorrectly change its uid and gid.
> That's buggy anyways, since it won't play well with /blah


Yep...



> > +++ ./pym/portage_locks.py      2005-11-16 01:56:25.152161768 -0500
> > @@ -358,3 +359,91 @@
> > +# should the fd be left open indefinitely?
> > +# IMO, it shouldn't, but opening/closing everytime around is expensive
> >
> > It should be closed when locks are released, I think. Otherwise the file may
> > be deleted and recreated while one FsLock instance has it open which could
> > create situations where two FsLocks are trying to lock the same path but
> > using different fds.
> Leave it open, on locking do a inode verification of the existing
> handle- if it's behind, close the fd, open, resume locking (offhand).
>
> Race potential, but that can be addressed by getting the read lock,
> then doing a second (or first, if it's not re-upping a lock) inode
> verification


Umm.. getting complicated. How often are locks actually obtained, released

and then obtained a second time? Sacrificing readability for performance in

corner cases doesn't seem quite right. Also, with a long running portage

process and badly written code, it'd be quite easy to run out of fds...



> >
> > +               if not blocking:
> > +                       try:    fcntl.flock(self.fd, flags|fcntl.LOCK_NB)
> > +                       except IOError, ie:
> > +                               if ie.errno == 11:
> >
> > 11 ?
> 42 is better? ;)
>
> Should be using errno.EAGAIN instead of hardcoding though...


Yep.



> > +       def __del__(self):
> > +               # alright, it's 5:45am, yes this is weird code.
> > +               try:
> > +                       if self.fd != None:
> > +                               self.release_read_lock()
> > +               finally:
> > +                       if self.fd != None:
> > +                               os.close(self.fd)
> >
> > Allowing an exception to be raised during __del__ is not a good thing...
>
> The reason it's left in is that-
> A) try/finally ensures despite whatever crazyness might occur with
> locking, the fd gets waxed
> B) blow ups in the code are visible, rather then silently swallowed
> C) exceptions in __del__ python complains about, but swallows.
>
> So... rather then silently ignoring any crazyness, it allows some
> noise to be made if the locking code does something weird, and also
> ensures the fd is closed.


No traceback though... Something like the following (on one line):



Exception exceptions.Exception:

<exceptions.Exception instance at 0x2aaaaab2db48> in

<bound method foo.__del__ of <__main__.foo instance at 0x2aaaaab2d5f0>>

ignored



I guess it's enough to point out where debugging needs to start...



> Should check self.fd.closed on a sidenote.
>
> > Any held lock should be cleaned up too otherwise misbehaving callers could
> > deadlock.
>
> Closing the fd == release of any locks that are bound to that fd.
> Hence the anal measures to make damn sure the fd gets closed :)


Ok. That works then.



> > +++ ./pym/portage_modules.py    2005-11-16 01:56:28.269687832 -0500


> > +               # * ferringb|afk hit it at some point, sure of that
> > +               # but no idea how, so commenting out to see if things break...
> > +               # except AttributeError, e:
> > +               #       raise FailedImport(name, e)
> >
> > I can't see how either, but comments like these don't really help. Please
> > leave them out unless they describe how the code works.
> Leaving comments regarding "weirdness occured via here, but went away"
> isn't a bad thing- if it reappears, you know that at least it was hit
> once prior.
>
> I know how it was hit also (just dawned on me how it would happen)-
> echo $'import os;print os.dar' > test.py
> python -c'import test'
>
> AttributeError...
>
> Should nuke the module imo in that case still, since it's a failed
> import.


Yep, so it shouldn't be commented then - which is my point. If code is to be

going into stable, things like this should be resolved prior to.



> > +++ ./pym/portage_plugins.py    2005-11-16 01:56:51.419168576 -0500
> > @@ -0,0 +1,174 @@
> > +# Copyright: 2005 Gentoo Foundation
> > +# License: GPL2
> > +# $Id:$
> > +
> > +# I don't like this.
> > +# doesn't seem clean/right.
> >
> > ???
> Someone didn't clean up the files prior to posting this...


Still not understanding, but as long as whatever it is it's addressed and

the comment removed...



> > +               # this could be fine grained down to per plugin_type
> > +               plug_lock = FsLock(plugins_dir)
> >
> > Then it probably should be. ;)
> It's not needed though.  Need a write lock on the plugins dir for when
> new plugin types are added; the only gain from it is when read locks
> are held long term, and parallel registration is occuring (for savior,
> it's possible, for stable, it isn't, not within the portage process at
> least).


Hrmm.. Then there should be a comment why the top-level plugins dir is

locked instead. Or perhaps there should be a global lock only while creating

the plugin_type subdir and then just locking the subdir afterward...



> >
> > +       def query_plugins(self, plugin_type=None, locking=True, raw=False):
> >
> > Not sure about allowing the external callers to disable locking... That
> > should only be used by register and deregister, right? Perhaps breaking
> > the unlocked version into a "protected" method?
>
> Debatable; any protection used people can stomp right past.  Not much
> for the "sitting on the porch with the shotgun" approach.
>
> Not really seeing the gain in hiding the locking tbh; shoving it into
> the instance namespace would require locking the attribute to maintain
> thread safety; the only gain is just an extra hoop to jump through if
> I'm after doing something stupid. :)


Should query_plugins be called without no locking in place? In the code so

far it is only called when a lock is being maintained outside of the method

call. I'm not sure what you are referring to with regard to instance

namespace. To be clear, I'm suggesting:



def register_plugins(...):

	lock_stuff()

	do_stuff()

	internal_query_plugins()

	unlock_stuff()



def query_plugins(...):

	lock_stuff()

	internal_query_plugins()

	unlock_stuff()



def internal_query_plugins(...):

	do_stuff()



There's no namespace stuff there...



> > +                                       d[x[:-len_exten]] = dict([(y, dict(c.items(y))) for y in c.sections()])
> >
> > Same here on the 2.2 bit... Although that's pretty hard to read either way.
> Prefer a map call? :)


Whatever is most readable (and supported).



> >
> > +               finally:
> > +                       if locking:
> > +                               plug_lock.release_read_lock()
> > +               if plugin_type != None:
> > +                       return d[plugin_type]
> > +               return d
> >
> > Not sure about having different return types...
> Agreed.


So one method for querying what plugin types are available and another for

querying plugins of a given type?



> > +registry = None
> > +from portage.util.currying import pre_curry, pretty_docs
> > +
> > +def proxy_it(method, *a, **kw):
> > +       global registry
> > +       if registry == None:
> > +               registry = GlobalPluginRegistry()
> > +       return getattr(registry, method)(*a, **kw)
> > +
> > +for x in ["register", "deregister", "query_plugins", "get_plugin"]:
> > +       v = pre_curry(proxy_it, x)
> > +       doc = getattr(GlobalPluginRegistry, x).__doc__
> > +       if doc == None:
> > +               doc = ''
> > +       else:
> > +               # do this so indentation on pydoc __doc__ is sane
> > +               doc = "\n".join(map(lambda x:x.lstrip(), doc.split("\n"))) +"\n"
> > +       doc += "proxied call to module level registry instances %s method" % x
> > +       globals()[x] = pretty_docs(v, doc)
> > +
> > +del x, v, proxy_it, doc
> >
> > What's this currying and pretty_docs stuff?
> currying == arg binding to a func- in this case, proxy_it gets first
> arg of the attr requested; the proxy_it func instantiates a
> GlobalPluginRegistry instance if it's missing.


This stuff seems to be missing from the patch then.



--

Jason Stubbs



-- 

gentoo-portage-dev@g.o mailing list