Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
On Tue, 2 Dec 2014, Dan Carpenter wrote: > On Mon, Dec 01, 2014 at 10:22:38PM +0100, SF Markus Elfring wrote: > > >> Which names would be better acceptable for you? > > > > > > You named it after the goto location but the label name should be based > > > on the label location to say what the goto does. > > > > I find it easier occasionally to name a label similarly to the jump target. > > That is a useless thing to do. > > > It seems that there are a few variations used for the affected identifiers. > > There is a lot of crap code in the kernel, yes. Does the label naming strategy appear in the conding style documentation anywhere? There are so many variants that just from looking at the code, it is hard to guess what is the best strategy. For example, out1, out2, etc are pretty uninformative, but they are concise and easy to spell correctly. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
On Mon, Dec 01, 2014 at 10:22:38PM +0100, SF Markus Elfring wrote: > >> Which names would be better acceptable for you? > > > > You named it after the goto location but the label name should be based > > on the label location to say what the goto does. > > I find it easier occasionally to name a label similarly to the jump target. That is a useless thing to do. > It seems that there are a few variations used for the affected identifiers. There is a lot of crap code in the kernel, yes. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
>> Which names would be better acceptable for you? > > You named it after the goto location but the label name should be based > on the label location to say what the goto does. I find it easier occasionally to name a label similarly to the jump target. It seems that there are a few variations used for the affected identifiers. > Something like "err_put_fsinfo", "err_put_fat", and "err_unload" or like that. How do you think about to provide a patch with your preferred names if my chances are lower to suggest the pleasing ones directly in my first tries? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
On Mon, Dec 01, 2014 at 04:30:44PM +0100, SF Markus Elfring wrote: > > Meanwhile Markus's error labels are absolutely useless. They give no > > indication of what going to the label does. > > Which names would be better acceptable for you? You named it after the goto location but the label name should be based on the label location to say what the goto does. Something like "err_put_fsinfo", "err_put_fat", and "err_unload" or like that. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
> Mixing the error paths together it always creates confusion. > I fix so many of these bugs... We get a few new ones every week. Would you like to get help here from any other automatic semantic patch approaches? > Meanwhile Markus's error labels are absolutely useless. They give no > indication of what going to the label does. Which names would be better acceptable for you? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
On Mon, 1 Dec 2014, Dan Carpenter wrote:
> On Sat, Nov 29, 2014 at 10:59:47PM +0900, OGAWA Hirofumi wrote:
> > Julia Lawall 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
On Sat, Nov 29, 2014 at 10:59:47PM +0900, OGAWA Hirofumi wrote: > Julia Lawall 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. Meanwhile Markus's error labels are absolutely useless. They give no indication of what going to the label does. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
Julia Lawall 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. (And we are initializing those for avoiding to be bothered by choosing correct label. If we really care micro optimize, initialization of those should not be required and should not be touched on other paths, and gcc can warn its usage.) Thanks. -- OGAWA Hirofumi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
On Sat, 29 Nov 2014, OGAWA Hirofumi wrote: > SF Markus Elfring writes: > > > From: Markus Elfring > > Date: Sat, 29 Nov 2014 07:37:34 +0100 > > > > The iput() function was called in an inefficient way by the implementation > > of the fat_fill_super() function in case of an allocation failure. > > The corresponding source code was improved by deletion of two unnecessary > > null pointer checks and a few adjustments for jump labels. > > 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. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] fs-fat: Less function calls in fat_fill_super() after error detection
SF Markus Elfring writes: > From: Markus Elfring > Date: Sat, 29 Nov 2014 07:37:34 +0100 > > The iput() function was called in an inefficient way by the implementation > of the fat_fill_super() function in case of an allocation failure. > The corresponding source code was improved by deletion of two unnecessary > null pointer checks and a few adjustments for jump labels. iput() checks NULL of inode. What is wrong just remove NULL check, instead of adding new jump labels? Thanks. -- OGAWA Hirofumi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
