Change in vdsm[master]: core: Add boot time to the getVdsStats API

nsoffer at redhat.com nsoffer at redhat.com
Fri Mar 28 12:50:53 UTC 2014


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 <dkuznets at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan 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