On Wed, Apr 21, 2010 at 00:09, Iustin Pop <[email protected]> wrote: > 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 >
Ah, now it's crystal clear. Thanks for the explanation Iustin. I'll fix it. /leczb
