<html><body><p>Brad,<br><br>Thanks for the review. Please see reply bellow:<br><br><tt>"openbmc" <openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org> wrote on 02/26/2016 08:52:10 PM:<br><br>> From: Brad Bishop <brad@bwbmail.net></tt><br><tt>> To: OpenBMC Patches <openbmc-patches@stwcx.xyz></tt><br><tt>> Cc: openbmc@lists.ozlabs.org</tt><br><tt>> Date: 02/26/2016 08:53 PM</tt><br><tt>> Subject: Re: [PATCH skeleton v3] skeleton: fixed hard-coding of <br>> master OCC hwmon sysfs path for PowerCap object</tt><br><tt>> Sent by: "openbmc" <openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org></tt><br><tt>> <br>> <br>> > + break<br>> > + else:<br>> > + raise dbus.exceptions.DBusException(<br>> > + "Cannot find user_powercap hwmon attribute, "+<br>> > + "check occ status.",<br>> > + name='org.freedesktop.DBus.Python.TypeError’)<br>> <br>> You don’t need a return statement after throwing an exception.<br>> <br>> > + return<br></tt><br><tt>OK. Will fix.</tt><br><br><tt>> > try:<br>> > - cmd_str = "echo "+str(value)+" > "+self.sysfs_attr<br>> > + cmd_str = 'echo '+str(value)+' > '+self.sysfs_attr<br>> > ret = subprocess.check_output(cmd_str, shell=True)<br>> > except subprocess.CalledProcessError as powerexc:<br>> > - print "Set PowerCap Error", powerexc.returncode,<br>> > - powerexc.output<br>> <br>> Would be nice to know if this was a real error or just user-error. <br>> Since this is external, user input, it probably makes sense to do <br>> all sorts of validation up front before passing it down lower in the<br>> stack rather than just relying on the called process to do it.<br>> <br></tt><br><tt>The reason I did not validate user input here, is the kernel driver itself (actually OCC itself) would respond wrong status</tt><br><tt>when setting invalid user power cap, e.g:</tt><br><br><tt>root@barreleye:/# echo 4000 > /sys/class/hwmon/hwmon3/user_powercap</tt><br><tt>occ-i2c 3-0050: Set User Powercap: wrong return status: 13</tt><br><tt>occ-i2c 3-0050: invalid powercap value: fa0</tt><br><tt>-sh: echo: write error: Invalid argument</tt><br><br><tt>But I agree that we need to put the policy in user space, rather than relying on kernel and OCC to handle error.</tt><br><br><tt>I will add user input validation here.</tt><br><br><tt>Thanks,</tt><br><tt>-Yi </tt><br><br><BR>
</body></html>