[PATCH skeleton v3] skeleton: fixed hard-coding of master OCC hwmon sysfs path for PowerCap object

[Yi TZ Li]

Brad,

Thanks for the review. Please see reply bellow:

"openbmc" <openbmc-bounces+shliyi=cn.ibm.com at lists.ozlabs.org> wrote on
02/26/2016 08:52:10 PM:

> From: Brad Bishop <brad at bwbmail.net>
> To: OpenBMC Patches <openbmc-patches at stwcx.xyz>
> Cc: openbmc at lists.ozlabs.org
> Date: 02/26/2016 08:53 PM
> Subject: Re: [PATCH skeleton v3] skeleton: fixed hard-coding of
> master OCC hwmon sysfs path for PowerCap object
> Sent by: "openbmc" <openbmc-bounces+shliyi=cn.ibm.com at lists.ozlabs.org>
>
>
> > +            break
> > +      else:
> > +         raise dbus.exceptions.DBusException(
> > +            "Cannot find user_powercap hwmon attribute, "+
> > +            "check occ status.",
> > +            name='org.freedesktop.DBus.Python.TypeError’)
>
> You don’t need a return statement after throwing an exception.
>
> > +         return

OK. Will fix.

> >       try:
> > -         cmd_str = "echo "+str(value)+" > "+self.sysfs_attr
> > +         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
>
> Would be nice to know if this was a real error or just user-error.
> Since this is external, user input, it probably makes sense to do
> all sorts of validation up front before passing it down lower in the
> stack rather than just relying on the called process to do it.
>

The reason I did not validate user input here, is the kernel driver itself
(actually OCC itself) would respond wrong status
when setting invalid user power cap, e.g:

root at barreleye:/# echo 4000 > /sys/class/hwmon/hwmon3/user_powercap
occ-i2c 3-0050: Set User Powercap: wrong return status: 13
occ-i2c 3-0050: invalid powercap value: fa0
-sh: echo: write error: Invalid argument

But I agree that we need to put the policy in user space, rather than
relying on kernel and OCC to handle error.

I will add user input validation here.

Thanks,
-Yi

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160301/b71efbb6/attachment.html>