| Message ID | d08248a2-46d8-6f73-81f4-84ad21b7e640@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Hi, On 12-03-18 22:23, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Mon, 12 Mar 2018 22:15:59 +0100 > > Adjust jump targets 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> So now we have a goto set_error_code with the code at the set_error_code doing something and then doing another goto. No, just no. goto-s going to a label calling another goto is completely unreadable. I really do not see any reason for the proposed changes, they may remove a small amount of code duplication, but at a hugh cost wrt readability. TL;DR: NACK Regards, Hans > --- > drivers/hwmon/sch5627.c | 60 ++++++++++++++++++++++++------------------------- > 1 file changed, 29 insertions(+), 31 deletions(-) > > diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c > index 91544f2312e6..e7a2a974ab74 100644 > --- a/drivers/hwmon/sch5627.c > +++ b/drivers/hwmon/sch5627.c > @@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, data); > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID); > - if (val < 0) { > - err = val; > - goto error; > - } > + if (val < 0) > + goto set_error_code; > + > if (val != SCH5627_HWMON_ID) { > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon", > val, SCH5627_HWMON_ID); > - err = -ENODEV; > - goto error; > + goto e_nodev; > } > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID); > - if (val < 0) { > - err = val; > - goto error; > - } > + if (val < 0) > + goto set_error_code; > + > if (val != SCH5627_COMPANY_ID) { > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company", > val, SCH5627_COMPANY_ID); > - err = -ENODEV; > - goto error; > + goto e_nodev; > } > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID); > - if (val < 0) { > - err = val; > - goto error; > - } > + if (val < 0) > + goto set_error_code; > + > if (val != SCH5627_PRIMARY_ID) { > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary", > val, SCH5627_PRIMARY_ID); > - err = -ENODEV; > - goto error; > + goto e_nodev; > } > > build_code = sch56xx_read_virtual_reg(data->addr, > SCH5627_REG_BUILD_CODE); > if (build_code < 0) { > err = build_code; > - goto error; > + goto remove_device; > } > > build_id = sch56xx_read_virtual_reg16(data->addr, > SCH5627_REG_BUILD_ID); > if (build_id < 0) { > err = build_id; > - goto error; > + goto remove_device; > } > > hwmon_rev = sch56xx_read_virtual_reg(data->addr, > SCH5627_REG_HWMON_REV); > if (hwmon_rev < 0) { > err = hwmon_rev; > - goto error; > + goto remove_device; > } > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL); > - if (val < 0) { > - err = val; > - goto error; > - } > + if (val < 0) > + goto set_error_code; > + > data->control = val; > if (!(data->control & 0x01)) { > pr_err("hardware monitoring not enabled\n"); > - err = -ENODEV; > - goto error; > + goto e_nodev; > } > /* Trigger a Vbat voltage measurement, so that we get a valid reading > the first time we read Vbat */ > @@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev) > */ > err = sch5627_read_limits(data); > if (err) > - goto error; > + goto remove_device; > > pr_info("found %s chip at %#hx\n", DEVNAME, data->addr); > pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n", > @@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev) > /* Register sysfs interface files */ > err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group); > if (err) > - goto error; > + goto remove_device; > > data->hwmon_dev = hwmon_device_register(&pdev->dev); > if (IS_ERR(data->hwmon_dev)) { > err = PTR_ERR(data->hwmon_dev); > data->hwmon_dev = NULL; > - goto error; > + goto remove_device; > } > > /* Note failing to register the watchdog is not a fatal error */ > @@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev) > > return 0; > > -error: > +set_error_code: > + err = val; > + goto remove_device; > + > +e_nodev: > + err = -ENODEV; > +remove_device: > sch5627_remove(pdev); > return err; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Adjust jump targets so that a bit of exception handling can be better >> reused at the end of this function. … > goto-s going to a label calling another goto is completely unreadable. I got an other software development view. > I really do not see any reason for the proposed changes, I suggest to look once more. > they may remove a small amount of code duplication, This was my software design goal in this case. > but at a hugh cost wrt readability. I proposed a different trade-off here. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 13-03-18 09:25, SF Markus Elfring wrote: >>> Adjust jump targets so that a bit of exception handling can be better >>> reused at the end of this function. > … >> goto-s going to a label calling another goto is completely unreadable. > > I got an other software development view. > > >> I really do not see any reason for the proposed changes, > > I suggest to look once more. > > >> they may remove a small amount of code duplication, > > This was my software design goal in this case. The diffstat of your patch is: 1 file changed, 29 insertions(+), 31 deletions(-) So you are asking people to review 60 changed lines to save 2, that alone should be the point where you stop yourself from *even* sending this patch. Review time is not an endless free resource and this patch is wasting it for very little gain. I see in the kernel git log that you've e.g. also send a lot of patches removing pr_err / dev_err calls from memory allocation failures. Those are good patches to have, they are easy to review and significantly shrink the compiled kernel size because of all the text strings you are removing. This patch however will likely result in a binary which is hardly smaller at all (if at all, the compiler does common code elimination itself) while it is a complex patch to review so comes with a large review cost. Next time before you send a patch please carefully think if the saving is worth the combination of reviewers time + the risk of regressions (and keep in mind that both the reviewers time and the risk of regressions cost increase for more complex changes). >> but at a hugh cost wrt readability. > > I proposed a different trade-off here. <sigh> not this again. Cleanup patches are appreciated, but there always is a cost to making changes to perfectly working good code. You've had this same discussion with Jonathan Cameron, the IIO subsys maintainer at the end of 2017, it would be nice if you were to actually listen to what people are saying about this. As for this specific discussion, there are certain "design-patterns" in the kernel, goto style error handling is one of them, the pattern there ALWAYS is: error_cleanup4: cleanup4(); /* fall through to next cleanup */ error_cleanup3: cleanup3(); /* fall through to next cleanup */ error_cleanup2: cleanup2(); /* fall through to next cleanup */ error_cleanup1: cleanup1(); /* fall through to next cleanup */ return error; Notice the fall-thoughs those are ALWAYS there, never, ever is there a goto after a cleanup label. The idea is that resources are allocated in the order of resource1 (cleaned by cleanup1()) then resource2, etc. and the goto + fallthrough will cleanup all resources which have been allocated at the time of the goto in *reverse* order of allocation. The whole point of this design- pattern to be able to a) do partial cleanup if we fail halfway through; b) do it in reverse order. Your patches black goto magic completely messes this up and clearly falls under the CS101 rule: never use goto. Your proposed changes break how every experienced kernel developer is expecting the code to be / work, causing your proposed changes to have a HUGE cost wrt maintainability and readability, which means the trade-off you're suggesting is anything but worth-while. TL;DR: still NACK. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 1 file changed, 29 insertions(+), 31 deletions(-) > > So you are asking people to review 60 changed lines to save 2, A bit of object code reduction might become useful also in this case. > that alone should be the point where you stop yourself from > *even* sending this patch. I proposed just another collateral evolution. > Next time before you send a patch please carefully think if the > saving is worth the combination of reviewers time + the risk of > regressions (and keep in mind that both the reviewers time and > the risk of regressions cost increase for more complex changes). Source code transformations were integrated in other software areas according to such a change pattern. > As for this specific discussion, there are certain "design-patterns" > in the kernel, goto style error handling is one of them, the pattern > there ALWAYS is: … > Notice the fall-thoughs those are ALWAYS there, never, ever is > there a goto after a cleanup label. It seems that I present an unusual update suggestion as a software design variant. > Your patches black goto magic completely messes this up You can view the proposal in such a way. > and clearly falls under the CS101 rule: never use goto. There might a target conflict with information from the section “7) Centralized exiting of functions” in the document “coding-style.rst”. Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 09:19:22AM +0100, Hans de Goede wrote: > Hi, > > On 12-03-18 22:23, SF Markus Elfring wrote: > >From: Markus Elfring <elfring@users.sourceforge.net> > >Date: Mon, 12 Mar 2018 22:15:59 +0100 > > > >Adjust jump targets 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> > > So now we have a goto set_error_code with the code at the set_error_code > doing something and then doing another goto. No, just no. goto-s going to > a label calling another goto is completely unreadable. > > I really do not see any reason for the proposed changes, they may remove > a small amount of code duplication, but at a hugh cost wrt readability. > > TL;DR: NACK > Fully agree. NACK confirmed. Guenter > Regards, > > Hans > > > > >--- > > drivers/hwmon/sch5627.c | 60 ++++++++++++++++++++++++------------------------- > > 1 file changed, 29 insertions(+), 31 deletions(-) > > > >diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c > >index 91544f2312e6..e7a2a974ab74 100644 > >--- a/drivers/hwmon/sch5627.c > >+++ b/drivers/hwmon/sch5627.c > >@@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, data); > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID); > >- if (val < 0) { > >- err = val; > >- goto error; > >- } > >+ if (val < 0) > >+ goto set_error_code; > >+ > > if (val != SCH5627_HWMON_ID) { > > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon", > > val, SCH5627_HWMON_ID); > >- err = -ENODEV; > >- goto error; > >+ goto e_nodev; > > } > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID); > >- if (val < 0) { > >- err = val; > >- goto error; > >- } > >+ if (val < 0) > >+ goto set_error_code; > >+ > > if (val != SCH5627_COMPANY_ID) { > > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company", > > val, SCH5627_COMPANY_ID); > >- err = -ENODEV; > >- goto error; > >+ goto e_nodev; > > } > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID); > >- if (val < 0) { > >- err = val; > >- goto error; > >- } > >+ if (val < 0) > >+ goto set_error_code; > >+ > > if (val != SCH5627_PRIMARY_ID) { > > pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary", > > val, SCH5627_PRIMARY_ID); > >- err = -ENODEV; > >- goto error; > >+ goto e_nodev; > > } > > build_code = sch56xx_read_virtual_reg(data->addr, > > SCH5627_REG_BUILD_CODE); > > if (build_code < 0) { > > err = build_code; > >- goto error; > >+ goto remove_device; > > } > > build_id = sch56xx_read_virtual_reg16(data->addr, > > SCH5627_REG_BUILD_ID); > > if (build_id < 0) { > > err = build_id; > >- goto error; > >+ goto remove_device; > > } > > hwmon_rev = sch56xx_read_virtual_reg(data->addr, > > SCH5627_REG_HWMON_REV); > > if (hwmon_rev < 0) { > > err = hwmon_rev; > >- goto error; > >+ goto remove_device; > > } > > val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL); > >- if (val < 0) { > >- err = val; > >- goto error; > >- } > >+ if (val < 0) > >+ goto set_error_code; > >+ > > data->control = val; > > if (!(data->control & 0x01)) { > > pr_err("hardware monitoring not enabled\n"); > >- err = -ENODEV; > >- goto error; > >+ goto e_nodev; > > } > > /* Trigger a Vbat voltage measurement, so that we get a valid reading > > the first time we read Vbat */ > >@@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev) > > */ > > err = sch5627_read_limits(data); > > if (err) > >- goto error; > >+ goto remove_device; > > pr_info("found %s chip at %#hx\n", DEVNAME, data->addr); > > pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n", > >@@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev) > > /* Register sysfs interface files */ > > err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group); > > if (err) > >- goto error; > >+ goto remove_device; > > data->hwmon_dev = hwmon_device_register(&pdev->dev); > > if (IS_ERR(data->hwmon_dev)) { > > err = PTR_ERR(data->hwmon_dev); > > data->hwmon_dev = NULL; > >- goto error; > >+ goto remove_device; > > } > > /* Note failing to register the watchdog is not a fatal error */ > >@@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev) > > return 0; > >-error: > >+set_error_code: > >+ err = val; > >+ goto remove_device; > >+ > >+e_nodev: > >+ err = -ENODEV; > >+remove_device: > > sch5627_remove(pdev); > > return err; > > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c index 91544f2312e6..e7a2a974ab74 100644 --- a/drivers/hwmon/sch5627.c +++ b/drivers/hwmon/sch5627.c @@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev) platform_set_drvdata(pdev, data); val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID); - if (val < 0) { - err = val; - goto error; - } + if (val < 0) + goto set_error_code; + if (val != SCH5627_HWMON_ID) { pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon", val, SCH5627_HWMON_ID); - err = -ENODEV; - goto error; + goto e_nodev; } val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID); - if (val < 0) { - err = val; - goto error; - } + if (val < 0) + goto set_error_code; + if (val != SCH5627_COMPANY_ID) { pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company", val, SCH5627_COMPANY_ID); - err = -ENODEV; - goto error; + goto e_nodev; } val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID); - if (val < 0) { - err = val; - goto error; - } + if (val < 0) + goto set_error_code; + if (val != SCH5627_PRIMARY_ID) { pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary", val, SCH5627_PRIMARY_ID); - err = -ENODEV; - goto error; + goto e_nodev; } build_code = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_BUILD_CODE); if (build_code < 0) { err = build_code; - goto error; + goto remove_device; } build_id = sch56xx_read_virtual_reg16(data->addr, SCH5627_REG_BUILD_ID); if (build_id < 0) { err = build_id; - goto error; + goto remove_device; } hwmon_rev = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_REV); if (hwmon_rev < 0) { err = hwmon_rev; - goto error; + goto remove_device; } val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL); - if (val < 0) { - err = val; - goto error; - } + if (val < 0) + goto set_error_code; + data->control = val; if (!(data->control & 0x01)) { pr_err("hardware monitoring not enabled\n"); - err = -ENODEV; - goto error; + goto e_nodev; } /* Trigger a Vbat voltage measurement, so that we get a valid reading the first time we read Vbat */ @@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev) */ err = sch5627_read_limits(data); if (err) - goto error; + goto remove_device; pr_info("found %s chip at %#hx\n", DEVNAME, data->addr); pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n", @@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev) /* Register sysfs interface files */ err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group); if (err) - goto error; + goto remove_device; data->hwmon_dev = hwmon_device_register(&pdev->dev); if (IS_ERR(data->hwmon_dev)) { err = PTR_ERR(data->hwmon_dev); data->hwmon_dev = NULL; - goto error; + goto remove_device; } /* Note failing to register the watchdog is not a fatal error */ @@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev) return 0; -error: +set_error_code: + err = val; + goto remove_device; + +e_nodev: + err = -ENODEV; +remove_device: sch5627_remove(pdev); return err; }