Nir Soffer has posted comments on this change.
Change subject: link monitor: replace events() with optionally continuous
iteration
......................................................................
Patch Set 15: Code-Review+1
(2 comments)
Overall this is great, but __iter__ starts with very ugly code for no reason.
+1 for the good parts.
....................................................
File lib/vdsm/ipwrapper.py
Line 619: def __iter__(self):
Line 620: try:
Line 621: self.proc.blocking = True
Line 622: except AttributeError:
Line 623: raise MonitorError('The monitor has not run yet')
You mix here two unrelated things:
- Assert that monitor was started
- Set proc.blocking to True
Your test of self.proc is not None is implicit, using unrelated side effect of
setting attributes on None. This is the worst way to say what you want to say
here.
The previous code was so clear:
if self.proc is None:
raise ...
self.proc.blocking = True
And it is also shorter 3 lines vs 4.
Line 624: for line in self.proc.stdout:
Line 625: yield self._parseLine(line)
Line 626:
Line 627: def start(self):
Line 621: self.proc.blocking = True
Line 622: except AttributeError:
Line 623: raise MonitorError('The monitor has not run yet')
Line 624: for line in self.proc.stdout:
Line 625: yield self._parseLine(line)
Perfect
Line 626:
Line 627: def start(self):
Line 628: self.proc = execCmd([_IP_BINARY.cmd, '-d', '-o', 'monitor',
'link'],
Line 629: sync=False)
--
To view, visit http://gerrit.ovirt.org/21430
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8ca850fc324a5b4c0268541fd4d4d062706a159
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]>
Gerrit-Reviewer: Assaf Muller <[email protected]>
Gerrit-Reviewer: Dan Kenigsberg <[email protected]>
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
