| Message ID | d496ca24-1725-768b-5e55-4e45097cb77d@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
>> 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
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
>>> They are both equally uninformative. >> >> Which identifier would you find appropriate there? > > error was fine. How do the different views fit together? Regards, Markus
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
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.
> 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
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; }