Skip to contentSkip to conversation

spca500: Use common error handling code in spca500_synch310()

PATCH In review SF Markus Elfring 4 participants 8 messages Sep 22, 2017

Subsystems (3)
Media (V4L/DVB) Kernel janitors Gspca USB webcam driver
lore
Share & tools
lore

SF Markus Elfring

author
#0 lore raw

PATCH 1/1 spca500: Use common error handling code in spca500_synch310()

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
 drivers/media/usb/gspca/spca500.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/media/usb/gspca/spca500.c +10 -11
index da2d9027914c..1f224f5e5b19 100644
@@ -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;
 }
Signature · 1 line
2.14.1
#1 lore raw
Quoted 37 lines · SF Markus Elfring

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
 drivers/media/usb/gspca/spca500.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/media/usb/gspca/spca500.c +4 -10
index da2d9027914c..1f224f5e5b19 100644
@@ -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)

drivers/media/usb/gspca/spca500.c static int spca500_synch310(struct gspca_dev *gspca_dev) · line 523

Quoted 10 lines
drivers/media/usb/gspca/spca500.c +6 -1
+	    < 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

author
#2 lore raw
Quoted 9 lines

drivers/media/usb/gspca/spca500.c static int spca500_synch310(struct gspca_dev *gspca_dev) · line 523

Quoted 7 lines
drivers/media/usb/gspca/spca500.c +3 -1
 	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.

Quoted 1 line

They are both equally uninformative.

Which identifier would you find appropriate there?

Regards,
Markus

#3 lore raw
Quoted 10 lines · SF Markus Elfring
Quoted 9 lines

drivers/media/usb/gspca/spca500.c static int spca500_synch310(struct gspca_dev *gspca_dev) · line 523

Quoted 7 lines
drivers/media/usb/gspca/spca500.c +3 -1
 	return 0;
-error:
+
+report_failure:
+	PERR("Set packet size: set interface error");
 	return -EBUSY;
 }

Why change the label name?

Quoted 6 lines

I find the suggested variant a bi better.

Quoted 1 line

They are both equally uninformative.

Which identifier would you find appropriate there?

error was fine.

julia

author
#4 lore raw
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

They are both equally uninformative.

Which identifier would you find appropriate there?

error was fine.

How do the different views fit together?

Regards,
Markus

#5 lore raw
Quoted 7 lines · SF Markus Elfring
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

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

author
#6 lore raw
Quoted 1 line

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.

Quoted 1 line

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?

Quoted 1 line

other than making the code harder to read.

I suggest to reconsider this concern.

Quoted 4 lines

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.

Quoted 1 line

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

#7 lore raw
In reply to SF Markus Elfring
Quoted 7 lines · SF Markus Elfring
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

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.