Change in vdsm[master]: Adding supervdsm unit tests

ybronhei at redhat.com ybronhei at redhat.com
Sun Oct 28 15:38:18 UTC 2012


Yaniv Bronhaim has posted comments on this change.

Change subject: Adding supervdsm unit tests
......................................................................


Patch Set 10: (3 inline comments)

....................................................
File vdsm/supervdsm.py
Line 114:         self._log.debug("Launching Super Vdsm")
Line 115:         superVdsmCmd = [constants.EXT_PYTHON, SUPERVDSM,
Line 116:                         self._authkey, str(os.getpid()),
Line 117:                         self.pidfile, self.timestamp, self.address,
Line 118:                         str(os.getuid())]
I'll add a comment that says:
'''
The creator of svdsm process passes to svdsm its internal files' names and the creator uid. It does that because when svdsm creates its internal files, it  also gives permissions to those files to allow the creator of svdsm to read them during its run. 
'''
Line 119:         misc.execCmd(superVdsmCmd, sync=False, sudo=True)
Line 120:         sleep(2)
Line 121: 
Line 122:     def kill(self):


....................................................
File vdsm/supervdsmServer.py
Line 326:         log = logging.getLogger("SuperVdsm.Server")
Line 327:         log.warn("Could not init proper logging", exc_info=True)
Line 328: 
Line 329:     log = logging.getLogger("SuperVdsm.Server")
Line 330: 
Why? The parts of code that are not inside of a try block can't throw exception anyway (almost all code is covered here).
If exception is raised, I catch it specifically and report related message.
Did I miss something? or it's just bad practice to split the code to small try blocks?
Line 331:     log.debug("Making sure I'm root")
Line 332:     if os.geteuid() != 0:
Line 333:         sys.exit(errno.EPERM)
Line 334: 


Line 371:             try:
Line 372:                 chown(f, int(uid), METADATA_GROUP)
Line 373:             except IOError:
Line 374:                 log.error("Could not change file permissions", exc_info=True)
Line 375:                 sys.exit(1)
If I got this straight, you say that if I exiting on exception its better to put all code inside big try block and if exception is raised we should print general message and exit? Isn't it better to be more specific to distinguish the error afterwards? or the trackback is enough?
Line 376: 
Line 377:         log.debug("Started serving super vdsm object")
Line 378:         servThread.join()
Line 379:     except Exception:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I08880b12db8a6ca9cc57d74d2ef2a86980a5b097
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>


More information about the vdsm-patches mailing list