Change in vdsm[master]: supervdsm: Fix race regarding instance ident

danken at redhat.com danken at redhat.com
Thu Feb 14 20:15:00 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: supervdsm: Fix race regarding instance ident
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File vdsm/supervdsm.py
Line 70:         self._supervdsmProxy = supervdsmProxy
Line 71: 
Line 72:     def __call__(self, *args, **kwargs):
Line 73:         def callMethod():
Line 74:             instance = self._supervdsmProxy.getInstance()
Ok. For some reason you do not want to let an exception leak out of getInstance, an return None. I'm following.
Line 75:             method = getattr(instance, self._funcName)
Line 76:             return method(*args, **kwargs)
Line 77: 
Line 78:         if not self._supervdsmProxy.isRunning():


Line 71: 
Line 72:     def __call__(self, *args, **kwargs):
Line 73:         def callMethod():
Line 74:             instance = self._supervdsmProxy.getInstance()
Line 75:             method = getattr(instance, self._funcName)
but now you get

 AttributeError: 'NoneType' object has no attribute 'bla'

and I am lost. I what way is this exception better than the originally swallowed one?
Line 76:             return method(*args, **kwargs)
Line 77: 
Line 78:         if not self._supervdsmProxy.isRunning():
Line 79:             # getting inside only when svdsm is down. its rare case so we


Line 198:         # We catch all exceptions as there is no documentation to what this
Line 199:         # mehthod throws normally.
Line 200:         except:
Line 201:             self._log.warn("Could not get instance, this is perfectly fine "
Line 202:                            "since this method was created just to catch this "
I suppose I should have seen my name written in acrostichon or something?
Line 203:                            "exception and return None in case the instance "
Line 204:                            "does not exist. Some people fear that one day "
Line 205:                            "there might be a bug in multiprocessing that "
Line 206:                            "causes the method to throw an exception that is "


--
To view, visit http://gerrit.ovirt.org/11759
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4f548cb9ad805508cf75df691e0ab58b3278dc87
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ohad Basan <obasan at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list