Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1737694 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] [media] spca500: Use common error handling code in  spca500_synch310() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-22 19:10 +0200
    Re: [PATCH] [media] spca500: Use common error handling code in  spca500_synch310() Julia Lawall <julia.lawall@lip6.fr> - 2017-09-22 19:10 +0200
      Re: [media] spca500: Use common error handling code in  spca500_synch310() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-22 19:50 +0200
        Re: [media] spca500: Use common error handling code in  spca500_synch310() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-22 19:50 +0200
          Re: [media] spca500: Use common error handling code in  spca500_synch310() Daniele Nicolodi <daniele@grinta.net> - 2017-09-22 20:40 +0200
            Re: [media] spca500: Use common error handling code in  spca500_synch310() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-09-22 22:50 +0200
          Re: [media] spca500: Use common error handling code in  spca500_synch310() Joe Perches <joe@perches.com> - 2017-09-22 20:40 +0200
        Re: [media] spca500: Use common error handling code in  spca500_synch310() Julia Lawall <julia.lawall@lip6.fr> - 2017-09-22 19:50 +0200

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

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

[toc] | [next] | [standalone]

#1737695

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

[toc] | [prev] | [next] | [standalone]

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

>>  	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

[toc] | [prev] | [next] | [standalone]

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

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

How do the different views fit together?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

> 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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web