On Thu, Jul 24, 2014 at 02:08:42PM -0400, Benjamin Romer wrote:
> +static ssize_t error_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u32 error;
> +
> + if (sscanf(buf, "%i\n", &error) == 1) {
> + if (visorchannel_write(ControlVm_channel,
> + offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
> + InstallationError),
> + &error, sizeof(u32)) == 1) {
visorchannel_write() returns either 0 or -EFAULT so this condition is
never true. This bug occurs in several places.
> + return count;
> + }
> + }
> + return -EIO;
> +}
It's almost always better to have error handling instead of success
handling. It should be a string of commands in a row and so we don't
have nested if statements.
ret = foo();
if (ret)
return ret;
ret = bar();
if (ret)
return ret;
ret = baz();
if (ret)
return ret;
ret = fizzbuzz();
if (ret)
return ret;
Look, Ma, no indenting! Also -EIO is wrong-ish. visorchannel_write()
should probably return -EIO instead of -EFAULT. Do it like this:
static ssize_t error_store(struct device *dev, struct device_attribute *attr,
const char *buf, size_t count)
{
u32 error;
int ret;
if (sscanf(buf, "%u\n", &error) != 1)
return -EINVAL;
ret = visorchannel_write(ControlVm_channel,
offsetof(ULTRA_CONTROLVM_CHANNEL_PROTOCOL,
InstallationError),
&error, sizeof(error));
if (ret)
return ret;
return count;
}
regards,
dan carpenter
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
