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

[Brian Harring]

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

>
> +                                       # 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

> +++ ./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

> +class FsLock(object):
> +       __slots__ = ["path", "fd", "create"]
> +       def __init__(self, path, create=False):
> +               """path specifies the fs path for the lock
> +               create controls whether the file will be create if the file doesn't exist
> +               if create is true, the base dir must exist, and it will create a file
> +               
> +               If you want a directory yourself, create it.
> +               """
> +               self.path = path
> +               self.fd = None
> +               self.create = create
> +               if not create:
> +                       if not os.path.exists(path):
> +                               raise NonExistant(path)
> +
> +       def _acquire_fd(self):
> +               if self.create:
> +                       try:    self.fd = os.open(self.path, os.R_OK|os.O_CREAT)
> +                       except OSError, oe:
> +                               raise GenericFailed(self.path, oe)
> +               else:
> +                       try:    self.fd = os.open(self.path, os.R_OK)
> +                       except OSError, oe:     raise NonExistant(self.path, oe)
> +       
> +       def _enact_change(self, flags, blocking):
> +               if self.fd == None:
> +                       self._acquire_fd()
> +               # we do it this way, due to the fact try/except is a bit of a hit
>
> Instead of?
Two fcntl calls instead of one.
It's minor offhand, just how it was originally written.

>
> +               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...

> +                                       return False
> +                               raise GenericFailed(self.path, ie)
> +               else:
> +                       fcntl.flock(self.fd, flags)
> +               return True
> +
> +       def acquire_write_lock(self, blocking=True):
> +               """Acquire an exclusive lock
> +               Returns True if lock is acquired, False if not.
> +               Note if you have a read lock, it implicitly upgrades atomically"""
> +               return self._enact_change(fcntl.LOCK_EX, blocking)
> +
> +       def acquire_read_lock(self, blocking=True):
> +               """Acquire a shared lock
> +               Returns True if lock is acquired, False if not.
> +               Note, if you have a write_lock already, it'll implicitly downgrade atomically"""
> +               return self._enact_change(fcntl.LOCK_SH, blocking)
> +       
> +       def release_write_lock(self):
> +               """Release an exclusive lock if held"""
> +               self._enact_change(fcntl.LOCK_UN, False)
> +               
> +       def release_read_lock(self):
> +               self._enact_change(fcntl.LOCK_UN, False)
> +
> +       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.

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

> +++ ./pym/portage_modules.py    2005-11-16 01:56:28.269687832 -0500
> +__import_lock = threading.Lock()
> +
> +def load_module(name):
> +       """load a module, throwing a FailedImport if __import__ fails"""
> +       __import_lock.acquire()
> +       try:
> +               if name in sys.modules:
> +                       return sys.modules[name]
> +               try:
> +                       m = __import__(name)
> +                       nl = name.split('.')
> +                       # __import__ returns nl[0]... so.
> +                       nl.pop(0)
> +                       while len(nl):
> +                               m = getattr(m, nl[0])
> +                               nl.pop(0)
> +                       return m
> +               # * 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.

> +++ ./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...

> +               # 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).

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


> Is [x for x in list] 2.2 safe by the way?
Yes.  The reversed in the code however is not.

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

>
> +               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.

> +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.

From there, pulls the request attribute, and passes the actual called 
args into the getattr'd func.

basically, a call to 
plugins.query_plugins(blah,a,sdf)
becomes
proxy_it("query_plugins", blah, a, sdf)

proxy_it raids the method name, getattr's the requested method (after 
instantiating if needed), then passes the remaining args/opt keywords 
to the method it retrieved.

Since curry'ing is basically wrapping/decorating the func, pretty_docs 
is used to rebind __doc__ string crap onto the func, so it actually 
has a sane docstring, rather then lacking any.
~harring