Nir Soffer has posted comments on this change. Change subject: core: Add boot time to the getVdsStats API ......................................................................
Patch Set 6: (2 comments) Code should be simplified and errors must be handled. http://gerrit.ovirt.org/#/c/25877/6/vdsm/sampling.py File vdsm/sampling.py: Line 151: def getBootTime(filepath=None): Line 152: if filepath is None: Line 153: filepath = '/proc/stat' Line 154: Line 155: with file(filepath) as f: We don't need the filepath arguemnt or temporary, just use literally use it: with file('/proc/stat') as f: ... Line 156: btime_line = [l for l in f.readlines() if l.startswith('btime')][0] Line 157: return int(btime_line.split()[-1]) Line 158: Line 159: Line 153: filepath = '/proc/stat' Line 154: Line 155: with file(filepath) as f: Line 156: btime_line = [l for l in f.readlines() if l.startswith('btime')][0] Line 157: return int(btime_line.split()[-1]) I don't see any need for a list comprehension here. I guess that we should have one such line, so this simple code is much more clear (it is also more efficient but nobody cares). The code should say - find the btime line and return the value. for line in f: if line.startswith('btime'): return int(line.split()[-1]) Now line.split()[-1] assumes that the format is "key value". If the format will change in the future (unlikely) to "key value1 value2", you will silently take the last value, instead of the first value. So more forward compatible code may be: line.split()[1] Finally we should handle the case where there is no such value. Your code will raise IndexError, which is not an error from the domain of time keeping. So either raise proper error, or use alternative method to get this value, like using /proc/uptime and current time. Line 158: Line 159: Line 160: class HostSample(BaseSample): Line 161: """ -- To view, visit http://gerrit.ovirt.org/25877 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I319e619cdaecac2f86d0154e3adbb3beda9c57d6 Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [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
