[PATCH skeleton v5] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object
Andrew Jeffery
andrew at aj.id.au
Mon Mar 7 12:41:25 AEDT 2016
Hi Yi Li,
On Fri, 2016-03-04 at 03:50 -0600, OpenBMC Patches wrote:
> From: Yi Li <adamliyi at msn.com>
>
> Currently master OCC hwmon sysfs path is hardcoded to '/sys/class/hwmon/hwmon3/'.
> This make the '/org/openbmc/sensors/host/PowerCap' interface cannot work when
> master OCC is initilized as hwmon device other than 'hwmon3'.
>
> 'usr_powercap' attribute should be searched when OCC is active.
> When skeleton creates 'PowerCap' objects, OCC may not be active.
> So do the search each time setting power cap. Raise an exception when
> there is no 'user_powercap' attribute.
> Fixes: openbmc/skeleton#42
>
> Also raise an exception when setting power cap fails. The exception
> will be returned to REST API caller. Need to specify the exception
> name to trigger a '400' return code.
> Fixes: openbmc/skeleton#43
>
> Add code to validate input user powercap value.
>
> Changed the logic of "reading powercap" using PowerCap.getValue().
> Previously PowerCap.getValue() returns current user set powercap value.
> Now PowerCap.getValue() returns actual system powercap reading from OCC.
Taking the commit message at face value it sounds like this should be
split into multiple commits.
>
> Signed-off-by: Yi Li <adamliyi at msn.com>
> ---
> bin/Sensors.py | 84 ++++++++++++++++++++++++++++++++++++++++++++------
> bin/sensor_manager2.py | 3 --
> 2 files changed, 75 insertions(+), 12 deletions(-)
>
> diff --git a/bin/Sensors.py b/bin/Sensors.py
> index f951eeb..6f5bc33 100755
> --- a/bin/Sensors.py
> +++ b/bin/Sensors.py
> @@ -1,13 +1,13 @@
> #!/usr/bin/python -u
>
> import sys
> -import subprocess
> #from gi.repository import GObject
> import gobject
> import dbus
> import dbus.service
> import dbus.mainloop.glib
> import os
> +import glob
> import Openbmc
>
> ## Abstract class, must subclass
> @@ -137,21 +137,87 @@ class PowerCap(VirtualSensor):
> > > def __init__(self, bus, name):
> > > > VirtualSensor.__init__(self, bus, name)
> > > > SensorValue.setValue(self, 0)
> -> > > self.sysfs_attr = "/sys/class/hwmon/hwmon3/user_powercap"
> > > ##override setValue method
> > > @dbus.service.method(SensorValue.IFACE_NAME,
> > > > in_signature='v', out_signature='')
> > > def setValue(self, value):
> > > > try:
> -> > > > cmd_str = "echo "+str(value)+" > "+self.sysfs_attr
> -> > > > ret = subprocess.check_output(cmd_str, shell=True)
> -> > > except subprocess.CalledProcessError as powerexc:
> -> > > > print "Set PowerCap Error", powerexc.returncode,
> -> > > > powerexc.output
> -> > > > return
> +> > > > value = int(value)
> +> > > except ValueError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Set an integer value",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +
> +> > > # master occ hwmon path is created dymanically
> +> > > # when occ is active scan hwmon sysfs directory for update
> +> > > for pcap_file in glob.glob('/sys/class/hwmon/*/user_powercap'):
> +> > > > occ_i2c_dev = os.path.dirname(pcap_file)+'/device'
Apologies for not raising some of these issues when I responded to your
initial patch. But:
os.path.join() gives us a way to avoid using string concatenation for
path building. Maybe we could do something like:
for pcap_file in glob.glob('/sys/class/hwmon/*/user_powercap'):
pcap_dir = os.path.dirname(pcap_file)
occ_i2c_dev = os.path.join(pcap_dir, "device")
> + occ_i2c_dev = os.path.realpath(
> +> > > > > > occ_i2c_dev).split('/').pop()
Again I think we could be taking better advantage of the os.path
methods - it has a basename() which I believe will provide what you
want:
occ_i2c_dev = os.path.basename(os.path.realpath(occ_i2c_dev))
> + if occ_i2c_dev == '3-0050':
> +> > > > > setcap_file = pcap_file
> +> > > > > break
> +> > > else:
So I learnt something new here - loops can have else cases[1]. It hurt
my brain a little but again [1] documents this approach as a good use
-case for the feature. Without knowing of the feature I probably
would've done something like:
from os import path as p # reduce verbosity for purpose of example
def upc_to_dev(user_powercap):
return p.realpath(p.join(p.dirname(user_powercap), 'device'))
def setValue(self, value):
...
upc_pattern = '/sys/class/hwmon/*/user_powercap'
setcap_files = [ x for x in glob.glob(upc_pattern)
if p.basename(upc_to_dev(x)) == '3-0050' ]
if not setcap_files:
raise dbusexceptions.DBusException(...)
setcap_file = setcap_files[0]
Does that improve the readability? Reduce it (e.g. use of list
comprehension)? Maybe I'm just bikeshedding here, but the for/else
concept was quite a surprise to me.
[1] http://python-notes.curiousefficiency.org/en/latest/python_concepts/break_else.html
> + raise dbus.exceptions.DBusException(
> +> > > > > "Cannot find user_powercap hwmon attribute, "+
> +> > > > > "check occ status.",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +
> +> > > # verify the user set value is in the range [min_cap, max_cap]
Can you please extract this out to a helper? Something like
def get_powercap_range(cap_file):
...
return min_cap, max_cap
> + try:
> +> > > > caps_file = open(os.path.dirname(setcap_file) +
> +> > > > > > '/caps_min_powercap', 'r')
Searching for 'close' gives no hits - we're leaking this resource?
Maybe we should be using contexts:
try:
with open(.../caps_min_powercap) as f_min:
with open(.../caps_max_powercap) as f_max:
return int(f_min.readline()), int(f_max.readline())
except IOError:
raise dbus.exceptions.DBusException(...)
Separately, os.path.join() again here, instead of string concat.
> + min_cap = caps_file.readline()
> +> > > > caps_file = open(os.path.dirname(setcap_file) +
> +> > > > > > '/caps_max_powercap', 'r')
> +> > > > max_cap = caps_file.readline()
> +> > > except IOError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot get caps_min or caps_max",
> +> > > > > name='org.freedesktop.DBus.Error.InvalidArgs')
> +
> + if value != 0 and (value < int(min_cap)
> +> > > > > or value > int(max_cap)):
This can just be:
min_cap, max_cap = get_powercap_range(setcaps_file)
if min_cap < value < max_cap:
for the condition, with
try:
with open(setcap_file, 'w') as f:
f.write(str(value))
except IOError:
raise dbus.exceptions.DBusException(...)
else:
print "Set PowerCap: ", value
SensorValue.setValue(self, value)
else:
raise dbus.exceptions.DBusException(...)
as the if/else bodies.
> + raise dbus.exceptions.DBusException(
> +> > > > > "Set a value in the range [%d, %d]"
> +> > > > > % (int(min_cap), int(max_cap)),
> +> > > > > name='org.freedesktop.DBus.Error.InvalidArgs')
> +
> +> > > try:
> +> > > > open(setcap_file, 'w').write(str(value));
Using the context as above prevents leaking the open file resource.
> + except IOError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot set PowerCap",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> > > > print "Set PowerCap: ", value
> > > > SensorValue.setValue(self, value)
> -> >
> +> > @dbus.service.method(SensorValue.IFACE_NAME,
> +> > > in_signature='', out_signature='v')
> +> > def getValue(self):
> +> > > # Read current powercap sensor. OCC will set system powercap
> +> > > # based on its algorithm
This is a copy/paste of the 'find-the-device' logic. Can we please
extract it to a helper method? This is partly resolved in my
alternative above.
> + for pcap_file in glob.glob(
> +> > > > > '/sys/class/hwmon/*/caps_curr_powercap'):
> +> > > > occ_i2c_dev = os.path.dirname(pcap_file)+'/device'
> +> > > > occ_i2c_dev = os.path.realpath(
> +> > > > > > occ_i2c_dev).split('/').pop()
> +> > > > if occ_i2c_dev == '3-0050':
> +> > > > > curr_cap_file = pcap_file
> +> > > > > break
> +> > > else:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot find caps_curr_powercap" +
> +> > > > > "hwmon attribute, " + "check occ status.",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +
> +> > > try:
> +> > > > cap_val = open(curr_cap_file, 'r').read()
Again, file isn't closed, probably best use contexts?
> + except IOError:
> +> > > > raise dbus.exceptions.DBusException(
> +> > > > > "Cannot read current powercap",
> +> > > > > name='org.freedesktop.DBus.Python.TypeError')
> +> > > return cap_val
This is fine as a str? Should it be an int?
> +
> class BootProgressSensor(VirtualSensor):
> > > def __init__(self,bus,name):
> > > > VirtualSensor.__init__(self,bus,name)
> diff --git a/bin/sensor_manager2.py b/bin/sensor_manager2.py
> index b5aac53..926bfea 100755
> --- a/bin/sensor_manager2.py
> +++ b/bin/sensor_manager2.py
> @@ -54,9 +54,6 @@ if __name__ == '__main__':
>
> > > obj_path = OBJ_PATH+"/host/PowerCap"
> > > sensor_obj = Sensors.PowerCap(bus,obj_path)
> -> > ## hwmon3 is default for master OCC on Barreleye.
> -> > ## should rewrite sensor_manager to remove hardcode
> -> > sensor_obj.sysfs_attr = "/sys/class/hwmon/hwmon3/user_powercap"
> > > root_sensor.add(obj_path,sensor_obj)
>
> > > obj_path = OBJ_PATH+"/host/BootProgress"
Cheers,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160307/6bcd81c9/attachment.sig>
More information about the openbmc
mailing list