Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection

Julia Lawall
2014-12-01T08:43:50Z

On Mon, 1 Dec 2014, Dan Carpenter wrote:

> On Sat, Nov 29, 2014 at 10:59:47PM +0900, OGAWA Hirofumi wrote:
> > Julia Lawall <julia.lawall@lip6.fr> writes:
> >
> > >> iput() checks NULL of inode. What is wrong just remove NULL check,
> > >> instead of adding new jump labels?
> > >
> > > Personally, I prefer that code that can be statically determined not to
> > > need to be executed not to be executed. It can make the code easier to
> > > understand, because each function is only called when doing so is useful,
> > > and it can be helpful to static analysis.
> >
> > Hm, first of all, we want to prevent the bugs. More labels are more
> > chances of bug (and we don't care micro optimize on this error path),
> > isn't it? Increasing the chance of bugs and bothers developers for
> > analyzer sounds like strange.
>
> Oh wow! Absolutely not. "One Err Bugs" are one of the most common
> kinds of bugs we have in the kernel. This is where you have just one
> error label and the bugs look like this:
>
> err:
> kfree(foo->bar);
> kfree(foo);
>
> but foo is NULL. Mixing the error paths together it always creates
> confusion. I fix so many of these bugs... We get a few new ones every
> week.

Just for concreteness, from drivers/clk/st/clkgen-mux.c (- indicates
lines of interest, not lines to remove):

@@ -722,7 +722,6 @@ void __init st_of_clkgen_vcc_setup(struc
return;

clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
- if (!clk_data)
goto err;

clk_data->clk_num = VCC_MAX_CHANNELS;
@@ -808,7 +807,6 @@ void __init st_of_clkgen_vcc_setup(struc
return;

err:
- for (i = 0; i < clk_data->clk_num; i++) {
struct clk_composite *composite;

if (!clk_data->clks[i])

julia