Dan Kenigsberg has posted comments on this change.
Change subject: Adding supervdsm unit tests
......................................................................
Patch Set 10: I would prefer that you didn't submit this
(4 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())]
would you remind me (and future reviewers - in a source comment) why passing
uid is important?
Could you achieve the same by having the test run supervdsm as non-root
(sudo=False)?
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:
this try-block is important, as it made sure that all exceptions are logged.
Removing it is wrong, as you need to replace it with multiple partial
try-blocks that logs exceptions.
If you do not like to "waste" an indentation level, you could break the
internals of the block into a helper function.
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)
you are existing on exception. so why bother to catch it at all?
Line 376:
Line 377: log.debug("Started serving super vdsm object")
Line 378: servThread.join()
Line 379: except Exception:
Line 380: log.error("Could not start Super Vdsm", exc_info=True)
Line 381: sys.exit(1)
Line 382: finally:
Line 383: try:
Line 384: if address is not None and os.path.exists(address):
address is never None. It was set in line 336. No need to check this case here
or in __pokeParent.
Line 385: os.unlink(address)
Line 386: except OSError:
Line 387: pass
Line 388:
--
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 <[email protected]>
Gerrit-Reviewer: Adam Litke <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Yaniv Bronhaim <[email protected]>
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
