On Tue, Apr 20, 2010 at 10:17:51PM +0100, Balazs Lecz wrote:
> On Tue, Apr 20, 2010 at 11:06, Iustin Pop <[email protected]> wrote:
>
> > On Tue, Apr 20, 2010 at 11:01:35AM +0100, Guido Trotter wrote:
> > > On Tue, Apr 20, 2010 at 10:59 AM, Iustin Pop <[email protected]> wrote:
> > >
> > > [+ balazs ]
> > >
> > > > On Mon, Apr 19, 2010 at 04:54:40PM +0100, Guido Trotter wrote:
> > > >> We can't guarantee to keep the LockedObject around, when ReleaseUid is
> > > >> called (because that might happen, for example, after a ganeti
> > > >> shutdown/restart). So we'll support releasing on just an integer uid.
> > > >>
> > > >> Signed-off-by: Guido Trotter <[email protected]>
> > > >> ---
> > > >> lib/uidpool.py | 8 ++++++--
> > > >> 1 files changed, 6 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/lib/uidpool.py b/lib/uidpool.py
> > > >> index 1ad9435..b1e2155 100644
> > > >> --- a/lib/uidpool.py
> > > >> +++ b/lib/uidpool.py
> > > >> @@ -322,9 +322,13 @@ def RequestUnusedUid(all_uids):
> > > >> def ReleaseUid(uid):
> > > >> """This should be called when the given user-id is no longer in
> > use.
> > > >>
> > > >> + @type uid: LockedUid or integer
> > > >> + @param uid: the uid to release back to the pool
> > > >> +
> > > >> """
> > > >> - # Make sure we release the exclusive lock, if there is any
> > > >> - uid.Unlock()
> > > >> + if isinstance(uid, LockedUid):
> > > >> + # Make sure we release the exclusive lock, if there is any
> > > >> + uid.Unlock()
> > > >> try:
> > > >> uid_path = utils.PathJoin(constants.UIDPOOL_LOCKDIR, str(uid))
> > > >> os.remove(uid_path)
> > > >
> > > > Hmm, I just realize that we rely on the str() representation to match
> > > > the uid number. Not optimal IMHO :), but anyway.
> > > >
> > >
> > > Yes, I don't like it either! :( Maybe we can improve this in a later
> > > patch series?
> > > Actually during testing of this patch series I changed the string
> > > representation to something which I thought was "nicer" and broke it
> > > already.
> >
> > Yeah, exactly, that's my point :), str is used for nice display.
> >
> > > So maybe it's something worth fixing! :)
> >
> > Indeed!
> >
> > iustin
> >
>
> I would be more than happy to fix this, but I don't see what needs to be
> fixed here.
s/__str__/AsString/ :)
> Don't get me wrong, I'm sure it's just me not understanding the problem.
> Please explain.
str() is used often for debugging, logging and other internals. It is
not customary to set its output in stone. E.g.:
logging.debug("Got uid %s", uid)
will use str() in the background. As such, I can expect Guido to have
extended it from str(self._uid) to (e.g.) "%s (created at %s)" %
(self._uid, self._ctime). Or annotating it via the method in which it
was created. Etc., etc..
> I had the following reasoning when I wrote the original code: a user-id is
> an integer by definition and its str() representation is expected to be the
> matching string of decimal digits (at least that's what I would expect). The
> filename should be UID_POOL_DIR + <uid>, where <uid> is the decimal
> representation of the user-id.
Yes, but you should do this via a defined API: AsStr(), or something
similar. Nowhere we declare that __str__ is a defined API of any class.
To recap:
- AsStr() or whatever other name returnin str(self._uid) - good, becase
we name that the official API
- __str__(): no, because __str__ is not our API, it's Python's
Consider it the same as too much operator overloading: handy, but
assumptions about operators can bite you hard.
regards,
iustin
--
Subscription settings:
http://groups.google.com/group/ganeti-devel/subscribe?hl=en
