Dan Kenigsberg has posted comments on this change.

Change subject: SuperVdsm validation of recovery after crash
......................................................................


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

(3 inline comments)

....................................................
File vdsm/supervdsm.py
Line 87:     A wrapper around all the supervdsm init stuff
Line 88:     """
Line 89:     _log = logging.getLogger("SuperVdsmProxy")
Line 90:     proxyLock = threading.Lock()
Line 91:     firstLaunch = True
I do not understand why you need this flag - and certainly not why it is 
class-wide. it should have been an instance variable.
Line 92: 
Line 93:     def open(self, *args, **kwargs):
Line 94:         return self._manager.open(*args, **kwargs)
Line 95: 


Line 130:                 createdTime = f.read().strip()
Line 131:         except IOError as e:
Line 132:             # pid file and timestamp file must be exist after first 
launch,
Line 133:             # otherwise excpetion will be raised to svdsm caller
Line 134:             if e.errno == ENOENT and self.firstLaunch:
it is expected that the files would be missing also after kill(), not only in 
firstLaunch.
Line 135:                 return False
Line 136: 
Line 137:         try:
Line 138:             pTime = str(misc.getProcCtime(spid))


Line 136: 
Line 137:         try:
Line 138:             pTime = str(misc.getProcCtime(spid))
Line 139:         except OSError:
Line 140:             # Means pid is not exist, svdsm was killed
I think you are not specific enough. OSError can mean a lot of other thing. You 
should catch only ESRCH.
Line 141:             return False
Line 142: 
Line 143:         if pTime == createdTime:
Line 144:             return True


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idad4a622b82259b777851d1b0c1b37ec8da2b01e
Gerrit-PatchSet: 26
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Eduardo <[email protected]>
Gerrit-Reviewer: Igor Lvovsky <[email protected]>
Gerrit-Reviewer: Royce Lv <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Shu Ming <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
Gerrit-Reviewer: Yeela Kaplan <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to