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

[Jason Stubbs]

On Saturday 19 November 2005 20:48, Brian Harring wrote:
> On Sat, Nov 19, 2005 at 01:12:46PM +0900, Jason Stubbs wrote:
> > > > +++ ./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.
>
> It's not really that complicated, and (imo) is a requisite if the
> locking obj is used for synchronizing during deletions;
>
> lock_obtained = False
> while not lock_obtained:
> 	if self.fd:
> 		self.read_lock()
> 		# check that the file didn't change under us
> 		if os.fstat(self.fd).st_ino != os.stat(self.path).st_ino):
> 			self.release_lock()
> 		else:
> 			lock_obtained = True
>
> 	if not self.fd:
> 		self._acquire_fd()
>
> Would be a rough example of it, bordering pseudocode (pardon bugs,
> 5:48am my time right now). :)
>
> EIther way, yes, it's a bit complicated, but locking isn't simple; as
> long as the api exported by the object is simple, handling the nasty
> crap internally shouldn't be an issue.

I'm not sure what use case you're talking about here... Hmm, actually yes
I do. So, you're right in that the above code is needed regardless, but...

> > Also, with a long running portage
> > process and badly written code, it'd be quite easy to run out of fds...
>
> All locking code (stable included) has this potential though- it's not
> something limited to this implementation.

I still don't see why fds should remain open after all locks are released. 
What my point above is about is that if some external process creates a bunch 
of FsLock objects that it keeps around to obtain future locks (bad design, 
but) fds will quickly become used up when they are not actually being used 
for anything.

> > > > +++ ./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.
>
> Code needs further work if it's going into stable imo.
>
> Reason I wanted the code split off from 3.0 and backported was to show
> a file based approach rather then trying to do scans at load up.

So, off topic then, eh? ;)  I should actually take the code for a spin then to 
see how it works.

> > > > +                                       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).
>
> Bit from above is supported in 2.2 and up.  Probably early, offhand.
> The example is a bit out of context, since the len_exten is defined
> above, and clear what it's doing- the dict arg instantiation might be
> a bit weird if people don't know that you can (essentially) pass a
> zipped set of arrays to dict, which it'll convert into key->vals...
> that said, chunking out a replacement of 3 lines seems a bit pointless
> imo.

Hmm.. I know the dict zipping bit, but no I can't see what it's doing. "x" is 
defined 6 lines above and seems to be a ptype. From what I know so far, I 
guess an example would be "cache"? "c" is defined 5 lines above and is an 
instance of RawConfigParser that has had read(os.path.join(plugins_dir, 
x.lstrip(os.path.sep))) called on it.

After reading over the patch in it's (incomplete ;) entirety, I'm guessing 
that "c" represents a file that defines what plugins have been registered for 
the current ptype. However, in this context the only indication of what that 
file contains is:

+ if raw:
+   d[x[:-len_exten]] = c
+ else:
+   d[x[:-len_exten]] = dict([(y, dict(c.items(y))) for y in c.sections()])

raw is a parameter/undocumented and d is the return type. This is what I mean 
by not readable. (Sorry to be harsh/dumb)

> > > > +               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?
>
> Or one for querying the full tree, one for pulling a specific
> branch...

Or all three? ;)

This is a difference in style, I guess. A list of plugin types plus a method 
to enumerate registered plugins for each type allows one to create a tree of 
plugins on one's own. Providing the tree of plugins means one has to extract 
information from a return and then throw away the rest. Performance-wise 
they're probably both about the same when talking the non-catered case...

> > > > +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.
>
> Offhand, looking over the split patch, this won't even work.
> The from portage.util.currying import ... is still savior namespace.
>
> Curious, qualms about using a massively cleaned up version of this?
> The reason I wanted this seperated out and backported was to go with
> an actual registry, and abuse the existing code- specific complaints
> with that route, beyond patch sucking?

None here. For what it's worth at this late stage, this patch is much better 
than my attempt. :)

--
Jason Stubbs

-- 
gentoo-portage-dev@g.o mailing list