From: Dan Carpenter <dan.carpenter@linaro.org>
To: Luca Ceresoli <luca.ceresoli@bootlin.com>
Cc: Markus Elfring <Markus.Elfring@web.de>,
linux-staging@lists.linux.dev, linux-tegra@vger.kernel.org,
linux-media@vger.kernel.org, kernel-janitors@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Hunter <jonathanh@nvidia.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Sowjanya Komatineni <skomatineni@nvidia.com>,
Thierry Reding <thierry.reding@gmail.com>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: staging: media: tegra-video: Use common error handling code in tegra_vi_graph_parse_one()
Date: Tue, 5 Mar 2024 19:21:44 +0300 [thread overview]
Message-ID: <e38cc9fb-9453-47ed-b460-335016581049@moroto.mountain> (raw)
In-Reply-To: <20240305162427.49a9f013@booty>
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
prev parent reply other threads:[~2024-03-05 16:21 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 18:55 [PATCH] staging: media: tegra-video: Use common error handling code in tegra_vi_graph_parse_one() Markus Elfring
2024-03-01 17:39 ` Luca Ceresoli
2024-03-02 9:30 ` Dan Carpenter
2024-03-02 10:40 ` Markus Elfring
2024-03-05 15:24 ` Luca Ceresoli
2024-03-05 16:21 ` Dan Carpenter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e38cc9fb-9453-47ed-b460-335016581049@moroto.mountain \
--to=dan.carpenter@linaro.org \
--cc=Markus.Elfring@web.de \
--cc=gregkh@linuxfoundation.org \
--cc=jonathanh@nvidia.com \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=linux-tegra@vger.kernel.org \
--cc=luca.ceresoli@bootlin.com \
--cc=mchehab@kernel.org \
--cc=skomatineni@nvidia.com \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).