Adam Litke has posted comments on this change.
Change subject: Adding supervdsm unit tests
......................................................................
Patch Set 9: I would prefer that you didn't submit this
(5 inline comments)
Only one remaining issue with this patch: use exc_info to log exceptions. -1
is not an insult to your patch. I like it, but there are just a few
improvements I'd like to see.
....................................................
File vdsm/supervdsm.py
Line 94: self._firstLaunch = True
Line 95:
Line 96: # Declaration of public variables that keep files' names that
svdsm
Line 97: # uses. We need to be able to change these variables so that
running
Line 98: # tests doesn't disturb and already running VDSM on the host.
Thanks. Looks good.
Line 99: self.pidfile = PIDFILE
Line 100: self.timestamp = TIMESTAMP
Line 101: self.address = ADDRESS
Line 102:
....................................................
File vdsm/supervdsmServer.py
Line 342: with open(pidfile, "w") as f:
Line 343: f.write(str(spid) + "\n")
Line 344: with open(timestamp, "w") as f:
Line 345: f.write(str(misc.getProcCtime(spid) + "\n"))
Line 346: except IOError as e:
you don't need 'as e' when using the logging exc_info method
Line 347: log.error("Could not write to svdsm internal files %s", e)
Line 348: sys.exit(1)
Line 349:
Line 350: log.debug("Cleaning old socket %s", address)
Line 343: f.write(str(spid) + "\n")
Line 344: with open(timestamp, "w") as f:
Line 345: f.write(str(misc.getProcCtime(spid) + "\n"))
Line 346: except IOError as e:
Line 347: log.error("Could not write to svdsm internal files %s", e)
log.error("Could not write to svdsm internal files", exc_info=True)
Line 348: sys.exit(1)
Line 349:
Line 350: log.debug("Cleaning old socket %s", address)
Line 351: if os.path.exists(address):
Line 360
Line 361
Line 362
Line 363
Line 364
Please use this block as an example for how the rest of your exception blocks
should be logging the errors. This is the only remaining issue I have with the
patch.
Line 370: for f in (address, timestamp, pidfile):
Line 371: try:
Line 372: chown(f, int(uid), METADATA_GROUP)
Line 373: except IOError as e:
Line 374: log.error("Could not change file %s permissions: %s",
f, e)
log.error("Could not change file %s permissions", f, exc_info=True)
Line 375: sys.exit(1)
Line 376:
Line 377: log.debug("Started serving super vdsm object")
Line 378: servThread.join()
--
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: 9
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
