| Message ID | dbebaea7-289c-47d9-ba06-cd58a10ea662@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | staging: media: tegra-video: Use common error handling code in tegra_vi_graph_parse_one() | expand |
Hello Markus, On Thu, 29 Feb 2024 19:55:46 +0100 Markus Elfring <Markus.Elfring@web.de> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Thu, 29 Feb 2024 19:44:36 +0100 > > Add a jump target so that a bit of exception handling can be better reused > at the end of this function implementation. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
On Fri, Mar 01, 2024 at 06:39:36PM +0100, Luca Ceresoli wrote: > Hello Markus, > > On Thu, 29 Feb 2024 19:55:46 +0100 > Markus Elfring <Markus.Elfring@web.de> wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Thu, 29 Feb 2024 19:44:36 +0100 > > > > Add a jump target so that a bit of exception handling can be better reused > > at the end of this function implementation. > > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> These patches make the code worse. If we're in the middle of a loop, then we should clean up the partial loop before doing the goto. Otherwise it creates a mess when we add a new allocation function after the end of the loop. Someone is going to add a _scoped() loop which uses cleanup.h magic to call _put automatically. This is a good option. regards, dan carpenter
>>> Add a jump target so that a bit of exception handling can be better reused >>> at the end of this function implementation. … >> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > These patches make the code worse. If we're in the middle of a loop, > then we should clean up the partial loop before doing the goto. > Otherwise it creates a mess when we add a new allocation function after > the end of the loop. How does such a feedback fit to another known information source? Section “7) Centralized exiting of functions” https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.8-rc6#n526 > Someone is going to add a _scoped() loop which uses cleanup.h magic to > call _put automatically. This is a good option. I became also curious how scope-based resource management will influence Linux coding styles further. Will various collateral evolution become more interesting? Regards, Markus
Hello Dan, Markus, On Sat, 2 Mar 2024 11:40:26 +0100 Markus Elfring <Markus.Elfring@web.de> wrote: > >>> Add a jump target so that a bit of exception handling can be better reused > >>> at the end of this function implementation. > … > >> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > These patches make the code worse. This is of course a legitimate opinion. However Markus' patch implements what is recommended by the documentation and is in common use in the kernel code. A quick search found 73 occurrences in v6.8-rc7: $ expr $(pcregrep -r -M ':\n\tfwnode_handle_put' drivers | wc -l) / 2 73 $ 300+ are found for of_node_put(). > > If we're in the middle of a loop, > > then we should clean up the partial loop before doing the goto. > > Otherwise it creates a mess when we add a new allocation function after > > the end of the loop. > > How does such a feedback fit to another known information source? > > Section “7) Centralized exiting of functions” > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.8-rc6#n526 > > > Someone is going to add a _scoped() loop which uses cleanup.h magic to > > call _put automatically. This is a good option. > > I became also curious how scope-based resource management will influence > Linux coding styles further. > Will various collateral evolution become more interesting? After some research I think I found what Dan means: https://lore.kernel.org/all/20240225142714.286440-3-jic23@kernel.org/ After reading the above thread, I agree using *_scoped() macros will be a good improvement. It is not yet in mainline as of v6.8-rc7, but it is in linux-next. So I think despite being valid this patch might still be discarded because a better solution should be available in a few weeks. Luca
On Tue, Mar 05, 2024 at 04:24:27PM +0100, Luca Ceresoli wrote: > Hello Dan, Markus, > > On Sat, 2 Mar 2024 11:40:26 +0100 > Markus Elfring <Markus.Elfring@web.de> wrote: > > > >>> Add a jump target so that a bit of exception handling can be better reused > > >>> at the end of this function implementation. > > … > > >> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > > > These patches make the code worse. > > This is of course a legitimate opinion. However Markus' patch > implements what is recommended by the documentation and is in common > use in the kernel code. A quick search found 73 occurrences in v6.8-rc7: > > $ expr $(pcregrep -r -M ':\n\tfwnode_handle_put' drivers | wc -l) / 2 > 73 > $ > > 300+ are found for of_node_put(). > Using an unwind ladder is the best way to write error handling, yes. I've written a long blog about it. https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/ In my blog, I talk about that "Unwinding from loops is slightly complicated." Because what you want to do is clean up partial iterations before the goto. Now imagine we apply Markus's patch and someone comes along an adds a new allocation after the loop is over. Then we have to do some kind of bunny hop: free_new_thing: free(thing); goto cleanup; <-- ugly little goto put_fwnode: fwnode_handle_put(remote); cleanup: dev_err(vi->dev, "failed parsing the graph: %d\n", ret); v4l2_async_nf_cleanup(&chan->notifier); return ret; Adding the little goto seems like a small thing when you're seeing it in an email like this. But when you add the new goto years later, people are used to unwind ladders working in a specific way and they forget that, "Oh this ladder has a weird rung that we have to skip over." We see these bugs more with locking. one = alloc(); if (!one) return; lock(); two = alloc(); if (!two) goto free_one; <-- should have unlocked before the goto unlock; three = alloc(); if (!three) goto free_two; return 0; free_two: free(two); free_one: unlock(); free(one); return -ENOMEM; regards, dan carpenter
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c index af6e3a0d8df4..5a08d9551f8b 100644 --- a/drivers/staging/media/tegra-video/vi.c +++ b/drivers/staging/media/tegra-video/vi.c @@ -1730,21 +1730,20 @@ static int tegra_vi_graph_parse_one(struct tegra_vi_channel *chan, ret = PTR_ERR(tvge); dev_err(vi->dev, "failed to add subdev to notifier: %d\n", ret); - fwnode_handle_put(remote); - goto cleanup; + goto put_fwnode; } ret = tegra_vi_graph_parse_one(chan, remote); - if (ret < 0) { - fwnode_handle_put(remote); - goto cleanup; - } + if (ret < 0) + goto put_fwnode; fwnode_handle_put(remote); } return 0; +put_fwnode: + fwnode_handle_put(remote); cleanup: dev_err(vi->dev, "failed parsing the graph: %d\n", ret); v4l2_async_nf_cleanup(&chan->notifier);