Change in vdsm[master]: vdsm: making PM 'on' and 'off' sync

emesika at redhat.com emesika at redhat.com
Sun Sep 21 08:06:13 UTC 2014


Eli Mesika has posted comments on this change.

Change subject: vdsm: making PM 'on' and 'off' sync
......................................................................


Patch Set 4:

(4 comments)

http://gerrit.ovirt.org/#/c/32695/4/vdsm/API.py
File vdsm/API.py:

Line 1227:         try:
Line 1228:             rc, out, err = fence(script, inp)
Line 1229:         except OSError as e:
Line 1230:             if e.errno == os.errno.ENOENT:
Line 1231:                 ret = errCode['fenceAgent']
> we create a regression here. we returned before, now we'll move on after th
Done
Line 1232:             else:
Line 1233:                 raise
Line 1234:                 self.log.debug('rc %s in %s out %s err %s', rc,
Line 1235:                                hidePasswd(inp), out, err)


Line 1231:                 ret = errCode['fenceAgent']
Line 1232:             else:
Line 1233:                 raise
Line 1234:                 self.log.debug('rc %s in %s out %s err %s', rc,
Line 1235:                                hidePasswd(inp), out, err)
> the debug log should be with 2 indentations back
why??? , it is fully aligned with 4 spaces exactly as it preceding line and it passes pep8
Line 1236:         if not 0 <= rc <= 2:
Line 1237:             ret = {'status': {'code': 1,
Line 1238:                               'message': out + err}}
Line 1239:         else:


Line 1232:             else:
Line 1233:                 raise
Line 1234:                 self.log.debug('rc %s in %s out %s err %s', rc,
Line 1235:                                hidePasswd(inp), out, err)
Line 1236:         if not 0 <= rc <= 2:
> i prefer - 
The condition as is is more readable as status return code should be between 0 and 2
Line 1237:             ret = {'status': {'code': 1,
Line 1238:                               'message': out + err}}
Line 1239:         else:
Line 1240:             message = doneCode['message']


Line 1236:         if not 0 <= rc <= 2:
Line 1237:             ret = {'status': {'code': 1,
Line 1238:                               'message': out + err}}
Line 1239:         else:
Line 1240:             message = doneCode['message']
> this can be the initial ret value instead of None and you can remove this e
didn't got you here , ret is initialized to None , this line sets the message .....
Line 1241:         if action == 'status':
Line 1242:             if rc == 0:
Line 1243:                 power = 'on'
Line 1244:             elif rc == 2:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I81d45977664c99c954c4663a26642e6e21cc65e3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eli Mesika <emesika at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Eli Mesika <emesika at redhat.com>
Gerrit-Reviewer: Martin Peřina <mperina at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list