From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx3.redhat.com (mx3.redhat.com [172.16.48.32]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id mBLKE1Bl010361 for ; Sun, 21 Dec 2008 15:14:01 -0500 Received: from smtp2-g19.free.fr (smtp2-g19.free.fr [212.27.42.28]) by mx3.redhat.com (8.13.8/8.13.8) with ESMTP id mBLKD4UL020561 for ; Sun, 21 Dec 2008 15:13:04 -0500 Message-ID: <494EA35F.10208@free.fr> Date: Sun, 21 Dec 2008 21:13:19 +0100 From: Thierry Merle MIME-Version: 1.0 To: Alexey Klimov References: <1229742563.10297.114.camel@tux.localhost> <20081220132730.45e9c365@gmail.com> <30353c3d0812200959h40d525f0t6939c21c6bd4e612@mail.gmail.com> <1229885212.12091.219.camel@tux.localhost> In-Reply-To: <1229885212.12091.219.camel@tux.localhost> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8bit Cc: video4linux-list@redhat.com, David Ellingsworth Subject: Re: [review patch 2/5] dsbr100: fix codinstyle, make ifs more clear List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: video4linux-list-bounces@redhat.com Errors-To: video4linux-list-bounces@redhat.com List-ID: 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 >> wrote: >>> Hello Alexey, >>> >>> On Sat, 20 Dec 2008 06:09:23 +0300 >>> Alexey Klimov 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; > } > ... > > 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