Re: [v2] [media] Use common error handling code in 20 functions

[SF Markus Elfring]

I got the following notification.

 - http://patchwork.linuxtv.org/patch/47270/
 - for: Linux Media kernel patches
was: New
now: Rejected


Would you like to clarify still any other variant for this change proposal?

Regards,
Markus

Re: [v2] [media] Use common error handling code in 20 functions

[Laurent Pinchart]

Hello,

On Wednesday, 28 February 2018 10:45:21 EET SF Markus Elfring wrote:
> >> +put_isp:
> >> +  omap3isp_put(video->isp);
> >> +delete_fh:
> >> +  v4l2_fh_del(&handle->vfh);
> >> +  v4l2_fh_exit(&handle->vfh);
> >> +  kfree(handle);
> > 
> > Please prefix the error labels with error_.
> 
> How often do you really need such an extra prefix?
> 
> >> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> >> @@ -994,10 +994,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
> >> void *fh, struct v4l2_queryctrl qc = { .id = ctrl->id };
> >> 
> >>ret = uvc_query_v4l2_ctrl(chain, &qc);
> >> 
> >> -  if (ret < 0) {
> >> -  ctrls->error_idx = i;
> >> -  return ret;
> >> -  }
> >> +  if (ret < 0)
> >> +  goto set_index;
> >> 
> >>ctrl->value = qc.default_value;
> >>
> >>}
> >> 
> >> @@ -1013,14 +1011,17 @@ static int uvc_ioctl_g_ext_ctrls(struct file
> >> *file,
> >> void *fh, ret = uvc_ctrl_get(chain, ctrl);
> >> 
> >>if (ret < 0) {
> >>
> >>uvc_ctrl_rollback(handle);
> >> 
> >> -  ctrls->error_idx = i;
> >> -  return ret;
> >> +  goto set_index;
> >> 
> >>}
> >>
> >>}
> >>
> >>ctrls->error_idx = 0;
> >>
> >>return uvc_ctrl_rollback(handle);
> >> 
> >> +
> >> +set_index:
> >> +  ctrls->error_idx = i;
> >> +  return ret;
> >> 
> >>  }
> > 
> > For uvcvideo I find this to hinder readability
> 
> I got an other development view.
> 
> > without adding much added value.
> 
> There can be a small effect for such a function implementation.
> 
> > Please drop the uvcvideo change from this patch.
> 
> Would it be nice if this source code adjustment could be integrated also?

Just for the record, and to avoid merging this patch by mistake,

Nacked-by: Laurent Pinchart 

at least until the requested changes are implemented.

-- 
Regards,

Laurent Pinchart

Re: [v2] [media] Use common error handling code in 20 functions

[SF Markus Elfring]

>> +put_isp:
>> +omap3isp_put(video->isp);
>> +delete_fh:
>> +v4l2_fh_del(&handle->vfh);
>> +v4l2_fh_exit(&handle->vfh);
>> +kfree(handle);
> 
> Please prefix the error labels with error_.

How often do you really need such an extra prefix?


>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -994,10 +994,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
>> void *fh, struct v4l2_queryctrl qc = { .id = ctrl->id };
>>
>>  ret = uvc_query_v4l2_ctrl(chain, &qc);
>> -if (ret < 0) {
>> -ctrls->error_idx = i;
>> -return ret;
>> -}
>> +if (ret < 0)
>> +goto set_index;
>>
>>  ctrl->value = qc.default_value;
>>  }
>> @@ -1013,14 +1011,17 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
>> void *fh, ret = uvc_ctrl_get(chain, ctrl);
>>  if (ret < 0) {
>>  uvc_ctrl_rollback(handle);
>> -ctrls->error_idx = i;
>> -return ret;
>> +goto set_index;
>>  }
>>  }
>>
>>  ctrls->error_idx = 0;
>>
>>  return uvc_ctrl_rollback(handle);
>> +
>> +set_index:
>> +ctrls->error_idx = i;
>> +return ret;
>>  }
> 
> For uvcvideo I find this to hinder readability

I got an other development view.


> without adding much added value.

There can be a small effect for such a function implementation.


> Please drop the uvcvideo change from this patch.

Would it be nice if this source code adjustment could be integrated also?

Regards,
Markus