[PATCH] [media] spca500: Use common error handling code in spca500_synch310()

[PATCH] [media] spca500: Use common error handling code in spca500_synch310()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 22 Sep 2017 18:45:07 +0200

Adjust a jump target so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/media/usb/gspca/spca500.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/gspca/spca500.c b/drivers/media/usb/gspca/spca500.c
index da2d9027914c..1f224f5e5b19 100644
--- a/drivers/media/usb/gspca/spca500.c
+++ b/drivers/media/usb/gspca/spca500.c
@@ -501,7 +501,6 @@ static int spca500_full_reset(struct gspca_dev *gspca_dev)
 static int spca500_synch310(struct gspca_dev *gspca_dev)
 {
-	if (usb_set_interface(gspca_dev->dev, gspca_dev->iface, 0) < 0) {
-		PERR("Set packet size: set interface error");
-		goto error;
-	}
+	if (usb_set_interface(gspca_dev->dev, gspca_dev->iface, 0) < 0)
+		goto report_failure;
+
 	spca500_ping310(gspca_dev);
@@ -514,12 +513,12 @@ static int spca500_synch310(struct gspca_dev *gspca_dev)
 	/* Windoze use pipe with altsetting 6 why 7 here */
-	if (usb_set_interface(gspca_dev->dev,
-				gspca_dev->iface,
-				gspca_dev->alt) < 0) {
-		PERR("Set packet size: set interface error");
-		goto error;
-	}
+	if (usb_set_interface(gspca_dev->dev, gspca_dev->iface, gspca_dev->alt)
+	    < 0)
+		goto report_failure;
+
 	return 0;
-error:
+
+report_failure:
+	PERR("Set packet size: set interface error");
 	return -EBUSY;
 }
 
-- 
2.14.1

Re: [PATCH] [media] spca500: Use common error handling code in spca500_synch310()

[Julia Lawall]

On Fri, 22 Sep 2017, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 22 Sep 2017 18:45:07 +0200
>
> Adjust a jump target so that a bit of exception handling can be better
> reused at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/media/usb/gspca/spca500.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/usb/gspca/spca500.c b/drivers/media/usb/gspca/spca500.c
> index da2d9027914c..1f224f5e5b19 100644
> --- a/drivers/media/usb/gspca/spca500.c
> +++ b/drivers/media/usb/gspca/spca500.c
> @@ -501,7 +501,6 @@ static int spca500_full_reset(struct gspca_dev *gspca_dev)
>  static int spca500_synch310(struct gspca_dev *gspca_dev)
>  {
> -	if (usb_set_interface(gspca_dev->dev, gspca_dev->iface, 0) < 0) {
> -		PERR("Set packet size: set interface error");
> -		goto error;
> -	}
> +	if (usb_set_interface(gspca_dev->dev, gspca_dev->iface, 0) < 0)
> +		goto report_failure;
> +
>  	spca500_ping310(gspca_dev);
> @@ -514,12 +513,12 @@ static int spca500_synch310(struct gspca_dev *gspca_dev)
>  	/* Windoze use pipe with altsetting 6 why 7 here */
> -	if (usb_set_interface(gspca_dev->dev,
> -				gspca_dev->iface,
> -				gspca_dev->alt) < 0) {
> -		PERR("Set packet size: set interface error");
> -		goto error;
> -	}
> +	if (usb_set_interface(gspca_dev->dev, gspca_dev->iface, gspca_dev->alt)
> +	    < 0)
> +		goto report_failure;
> +
>  	return 0;
> -error:
> +
> +report_failure:
> +	PERR("Set packet size: set interface error");
>  	return -EBUSY;
>  }

Why change the label name?  They are both equally uninformative.

julia

Re: [media] spca500: Use common error handling code in spca500_synch310()

[SF Markus Elfring]

>>  	return 0;
>> -error:
>> +
>> +report_failure:
>> +	PERR("Set packet size: set interface error");
>>  	return -EBUSY;
>>  }
> 
> Why change the label name?

I find the suggested variant a bi better.


> They are both equally uninformative.

Which identifier would you find appropriate there?

Regards,
Markus

Re: [media] spca500: Use common error handling code in spca500_synch310()

[Julia Lawall]

On Fri, 22 Sep 2017, SF Markus Elfring wrote:

> >>  	return 0;
> >> -error:
> >> +
> >> +report_failure:
> >> +	PERR("Set packet size: set interface error");
> >>  	return -EBUSY;
> >>  }
> >
> > Why change the label name?
>
> I find the suggested variant a bi better.
>
>
> > They are both equally uninformative.
>
> Which identifier would you find appropriate there?

error was fine.

julia

Re: [media] spca500: Use common error handling code in spca500_synch310()

[SF Markus Elfring]

>>> They are both equally uninformative.
>>
>> Which identifier would you find appropriate there?
> 
> error was fine.

How do the different views fit together?

Regards,
Markus

Re: [media] spca500: Use common error handling code in spca500_synch310()

[Daniele Nicolodi]

On 9/22/17 11:46 AM, SF Markus Elfring wrote:
>>>> They are both equally uninformative.
>>>
>>> Which identifier would you find appropriate there?
>>
>> error was fine.
> 
> How do the different views fit together?

You want to change something.  Changing something requires to spend
energy.  You need to to justify why spending that energy is a good
thing.  No one needs to argue about keeping it the way it is.

What about stopping changing code for the sake of having one more patch
accepted in the kernel?  I don't see any improvement brought by the
proposed change, other than making the code harder to read.  I find goto
statements hard to read, because they inherently make some information
non local.  They are justified in error path handling, if the error path
only unwinds what the function did early on.  That's not the case here.

The same applies to dozens of patches you proposed recently.

By the way, there may be some useful absent minded code churn of the
king you like in that driver: I don't think the PERR macro is the
idiomatic way of doing logging.

Cheers,
Daniele

Re: [media] spca500: Use common error handling code in spca500_synch310()

[Joe Perches]

On Fri, 2017-09-22 at 19:46 +0200, SF Markus Elfring wrote:
> > > > They are both equally uninformative.
> > > 
> > > Which identifier would you find appropriate there?
> > 
> > error was fine.
> 
> How do the different views fit together?

Markus, please respect what others tell you because
your coding style "taste" could be improved.

Re: [media] spca500: Use common error handling code in spca500_synch310()

[SF Markus Elfring]

> No one needs to argue about keeping it the way it is.

I got an other impression in this case after a bit of information
was presented which seems to be contradictory.


> I don't see any improvement brought by the proposed change,

Do you care if the source code for an error message is present only once
in this function?


> other than making the code harder to read.

I suggest to reconsider this concern.


> I find goto statements hard to read, because they inherently make some
> information non local.  They are justified in error path handling,
> if the error path only unwinds what the function did early on.
> That's not the case here.

I am looking also for change possibilities without such a restriction.

 
> The same applies to dozens of patches you proposed recently.

I proposed some software updates to reduce a bit of code duplication.

Do you find any corresponding approaches useful?

Regards,
Markus