Hi Eduardo,
Douglas pointed out to me that I hadn't reviewed this series yet.
That was mostly because it's pretty good as far as I'm concerned :-)
I do think one small thing should change:
On Monday 27 July 2009 17:12:08 Eduardo Valentin wrote:
> diff --git a/linux/drivers/media/radio/si4713-i2c.c
> b/linux/drivers/media/radio/si4713-i2c.c
<snip>
> +/* write string property */
> +static int si4713_write_econtrol_string(struct si4713_device *sdev,
> + struct v4l2_ext_control *control)
> +{
> + struct v4l2_queryctrl vqc;
> + int len;
> + s32 rval = 0;
> +
> + vqc.id = control->id;
> + rval = si4713_queryctrl(&sdev->sd, &vqc);
> + if (rval < 0)
> + goto exit;
> +
> + switch (control->id) {
> + case V4L2_CID_RDS_TX_PS_NAME: {
> + char ps_name[MAX_RDS_PS_NAME + 1];
> +
> + len = control->size - 1;
> + if (len > MAX_RDS_PS_NAME)
> + len = MAX_RDS_PS_NAME;
> + rval = copy_from_user(ps_name, control->string, len);
> + if (rval < 0)
> + goto exit;
> + ps_name[len] = '\0';
> +
> + if (strlen(ps_name) % vqc.step) {
> + rval = -EINVAL;
I think we should return -ERANGE instead. It makes more sense than -EINVAL,
since it is the string length that is out of bounds. -ERANGE is the expected
error code when the control boundary checks fail.
I know I said EINVAL before, but after thinking about it some more I've
reconsidered.
> + goto exit;
> + }
> +
> + rval = si4713_set_rds_ps_name(sdev, ps_name);
> + }
> + break;
> +
> + case V4L2_CID_RDS_TX_RADIO_TEXT:{
> + char radio_text[MAX_RDS_RADIO_TEXT + 1];
> +
> + len = control->size - 1;
> + if (len > MAX_RDS_RADIO_TEXT)
> + len = MAX_RDS_RADIO_TEXT;
> + rval = copy_from_user(radio_text, control->string, len);
> + if (rval < 0)
> + goto exit;
> + radio_text[len] = '\0';
> +
> + if (strlen(radio_text) % vqc.step) {
> + rval = -EINVAL;
Ditto.
> + goto exit;
> + }
> +
> + rval = si4713_set_rds_radio_text(sdev, radio_text);
> + }
> + break;
> +
> + default:
> + rval = -EINVAL;
> + break;
> + };
> +
> +exit:
> + return rval;
> +}
Just change this and you can add
Reviewed-by: Hans Verkuil <hverk...@xs4all.nl>
for the whole series.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
