Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear

[Thierry Merle <thierry.merle@free.fr>]

Hello,

Alexey Klimov a écrit :
> On Sat, 2008-12-20 at 12:59 -0500, David Ellingsworth wrote:
>> On Sat, Dec 20, 2008 at 10:27 AM, Douglas Schilling Landgraf
>> <dougsland@gmail.com> wrote:
>>> Hello Alexey,
>>>
>>> On Sat, 20 Dec 2008 06:09:23 +0300
>>> Alexey Klimov <klimov.linux@gmail.com> wrote:
>>>
>>>> We should make if-constructions more clear. Introduce int variables in
>>>> some functions to make it this way.
>>>>
>>>> ---
>>>> diff -r a302bfcb23f8 linux/drivers/media/radio/dsbr100.c
>>>> --- a/linux/drivers/media/radio/dsbr100.c     Fri Dec 19 14:34:30
>>>> 2008 +0300 +++ b/linux/drivers/media/radio/dsbr100.c  Sat Dec
>>>> 20 02:31:26 2008 +0300 @@ -200,15 +200,24 @@
>>>>  /* switch on radio */
>>>>  static int dsbr100_start(struct dsbr100_device *radio)
>>>>  {
>>>> +     int first;
>>>> +     int second;
>>>> +
>>>>       mutex_lock(&radio->lock);
>>>> -     if (usb_control_msg(radio->usbdev,
>>>> usb_rcvctrlpipe(radio->usbdev, 0),
>>>> -                     USB_REQ_GET_STATUS,
>>>> -                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
>>>> USB_DIR_IN,
>>>> -                     0x00, 0xC7, radio->transfer_buffer, 8, 300)
>>>> < 0 ||
>>>> -     usb_control_msg(radio->usbdev,
>>>> usb_rcvctrlpipe(radio->usbdev, 0),
>>>> -                     DSB100_ONOFF,
>>>> -                     USB_TYPE_VENDOR | USB_RECIP_DEVICE |
>>>> USB_DIR_IN,
>>>> -                     0x01, 0x00, radio->transfer_buffer, 8, 300)
>>>> < 0) { +
>>>> +     first = usb_control_msg(radio->usbdev,
>>>> +             usb_rcvctrlpipe(radio->usbdev, 0),
>>>> +             USB_REQ_GET_STATUS,
>>>> +             USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>>>> +             0x00, 0xC7, radio->transfer_buffer, 8, 300);
>>>> +
>>>> +     second = usb_control_msg(radio->usbdev,
>>>> +             usb_rcvctrlpipe(radio->usbdev, 0),
>>>> +             DSB100_ONOFF,
>>>> +             USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>>>> +             0x01, 0x00, radio->transfer_buffer, 8, 300);
>>>> +
>>>> +     if (first < 0 || second < 0) {
>>>>               mutex_unlock(&radio->lock);
>>>>               return -1;
>>>>       }
>>> IMO, we could create a variable like "ret" or "retval" to validate each
>>> usb_control_msg call instead of create 3 variables "first", "second" and "third".
>> The primary problem I have with this patch is that it changes the
>> behavior of the driver. The original way it was written the function
>> would immediately return if one of the calls to usb_control_msg
>> failed. With this patch, if the first call fails it will still make a
>> second call to usb_control_msg.
>>
>> I agree with Douglas, a single "ret" variable should be used and then
>> evaluated after every usb_control_msg call.
>>
>> [snip]
>>
>> Regards,
>>
>> David Ellingsworth
> 
> Hello, all
> Yes, you are right, my bad - i missed that thing.
> 
> Also, it's better to add dev_err messages that reports in case of
> retval<0 what usb_control_msg returned, request and what function it
> was.
> 
> I suppose example could look like this:
> 
> static int dsbr100_start(struct dsbr100_device *radio)
> {
>         int retval;
> 
>         mutex_lock(&radio->lock);
> 
>         retval = usb_control_msg(radio->usbdev,
>                 usb_rcvctrlpipe(radio->usbdev, 0),
>                 USB_REQ_GET_STATUS,
>                 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>                 0x00, 0xC7, radio->transfer_buffer, 8, 300);
> 
>         if (retval < 0) {
>                 mutex_unlock(&radio->lock);
>                 dev_err(&radio->usbdev->dev,
>                         "usb_control_msg returned %i, request %i in %s\n",
> 				retval, USB_REQ_GET_STATUS, __func__);
>                 return -1;
>         }
> 
>         retval = usb_control_msg(radio->usbdev,
>                 usb_rcvctrlpipe(radio->usbdev, 0),
>                 DSB100_ONOFF,
>                 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>                 0x01, 0x00, radio->transfer_buffer, 8, 300);
> 
>         if (retval < 0) {
>                 mutex_unlock(&radio->lock);
>                 dev_err(&radio->usbdev->dev,
>                         "usb_control_msg returned %i, request %i in %s\n",
> 				retval, DSB100_ONOFF, __func__);
>                 return -1;
>         }
> ...<snip>
> 
> But it looks, that more comfortable to create macro, may be smth that looks like:
> 
> #define dsbr_usb_control_msg_err(arg)                                   \
>                 dev_err(&radio->usbdev->dev,                    	\
>                         "%s - usb_control_msg returned %i, request %i\n",\
>                                 __func__, retval, arg)
> 
> And then i can use: 
> 
>         retval = usb_control_msg(radio->usbdev,
>                 usb_rcvctrlpipe(radio->usbdev, 0),
>                 USB_REQ_GET_STATUS,
>                 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>                 0x00, 0xC7, radio->transfer_buffer, 8, 300);
> 
>         if (retval < 0) {
>                 mutex_unlock(&radio->lock);
>                 dsbr_usb_control_msg_err(USB_REQ_GET_STATUS);
>                 return -1;
>         }
> 
>         retval = usb_control_msg(radio->usbdev,
>                 usb_rcvctrlpipe(radio->usbdev, 0),
>                 DSB100_ONOFF,
>                 USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_IN,
>                 0x01, 0x00, radio->transfer_buffer, 8, 300);
> 
>         if (retval < 0) {
>                 mutex_unlock(&radio->lock);
>                 dsbr_usb_control_msg_err(DSB100_ONOFF);
>                 return -1;
>         }
> 
> We should see what function and request failed.
> I didn't mean that this is right thing, it's just approach, that i can
> offer.
> 
> What do you think ?
> 
> 
I would use a goto.
This is the most readable and efficient way to manage exeptions in C.
Take a look at linux/drivers/media/video/v4l2-dev.c for an example of goto usage.
Cheers,
Thierry

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list