From: Markus Elfring <[email protected]>
Date: Sat, 15 Nov 2014 20:55:23 +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.
Signed-off-by: Markus Elfring <[email protected]>
---
fs/fat/inode.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 756aead..138ab9a 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1716,20 +1716,20 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
fsinfo_inode = new_inode(sb);
if (!fsinfo_inode)
- goto out_fail;
+ goto fsinfo_inode_failure;
fsinfo_inode->i_ino = MSDOS_FSINFO_INO;
sbi->fsinfo_inode = fsinfo_inode;
insert_inode_hash(fsinfo_inode);
root_inode = new_inode(sb);
if (!root_inode)
- goto out_fail;
+ goto other_failure;
root_inode->i_ino = MSDOS_ROOT_INO;
root_inode->i_version = 1;
error = fat_read_root(root_inode);
if (error < 0) {
iput(root_inode);
- goto out_fail;
+ goto other_failure;
}
error = -ENOMEM;
insert_inode_hash(root_inode);
@@ -1737,7 +1737,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
sb->s_root = d_make_root(root_inode);
if (!sb->s_root) {
fat_msg(sb, KERN_ERR, "get root inode failed");
- goto out_fail;
+ goto other_failure;
}
if (sbi->options.discard) {
@@ -1756,11 +1756,13 @@ out_invalid:
if (!silent)
fat_msg(sb, KERN_INFO, "Can't find a valid FAT filesystem");
+other_failure:
+ iput(fsinfo_inode);
+
+fsinfo_inode_failure:
+ iput(fat_inode);
+
out_fail:
- if (fsinfo_inode)
- iput(fsinfo_inode);
- if (fat_inode)
- iput(fat_inode);
unload_nls(sbi->nls_io);
unload_nls(sbi->nls_disk);
if (sbi->options.iocharset != fat_default_iocharset)
--
2.1.3
On Sat, 15 Nov 2014, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Sat, 15 Nov 2014 20:55:23 +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.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> fs/fat/inode.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 756aead..138ab9a 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -1716,20 +1716,20 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
>
> fsinfo_inode = new_inode(sb);
> if (!fsinfo_inode)
> - goto out_fail;
> + goto fsinfo_inode_failure;
> fsinfo_inode->i_ino = MSDOS_FSINFO_INO;
> sbi->fsinfo_inode = fsinfo_inode;
> insert_inode_hash(fsinfo_inode);
>
> root_inode = new_inode(sb);
> if (!root_inode)
> - goto out_fail;
> + goto other_failure;
Other_failure is not such a good name. The one above is better.
julia
> root_inode->i_ino = MSDOS_ROOT_INO;
> root_inode->i_version = 1;
> error = fat_read_root(root_inode);
> if (error < 0) {
> iput(root_inode);
> - goto out_fail;
> + goto other_failure;
> }
> error = -ENOMEM;
> insert_inode_hash(root_inode);
> @@ -1737,7 +1737,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
> sb->s_root = d_make_root(root_inode);
> if (!sb->s_root) {
> fat_msg(sb, KERN_ERR, "get root inode failed");
> - goto out_fail;
> + goto other_failure;
> }
>
> if (sbi->options.discard) {
> @@ -1756,11 +1756,13 @@ out_invalid:
> if (!silent)
> fat_msg(sb, KERN_INFO, "Can't find a valid FAT filesystem");
>
> +other_failure:
> + iput(fsinfo_inode);
> +
> +fsinfo_inode_failure:
> + iput(fat_inode);
> +
> out_fail:
> - if (fsinfo_inode)
> - iput(fsinfo_inode);
> - if (fat_inode)
> - iput(fat_inode);
> unload_nls(sbi->nls_io);
> unload_nls(sbi->nls_disk);
> if (sbi->options.iocharset != fat_default_iocharset)
> --
> 2.1.3
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
From: Markus Elfring <[email protected]>
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.
Signed-off-by: Markus Elfring <[email protected]>
---
fs/fat/inode.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 756aead..a39afe8 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1716,20 +1716,20 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
fsinfo_inode = new_inode(sb);
if (!fsinfo_inode)
- goto out_fail;
+ goto fsinfo_inode_failure;
fsinfo_inode->i_ino = MSDOS_FSINFO_INO;
sbi->fsinfo_inode = fsinfo_inode;
insert_inode_hash(fsinfo_inode);
root_inode = new_inode(sb);
if (!root_inode)
- goto out_fail;
+ goto failure_exit;
root_inode->i_ino = MSDOS_ROOT_INO;
root_inode->i_version = 1;
error = fat_read_root(root_inode);
if (error < 0) {
iput(root_inode);
- goto out_fail;
+ goto failure_exit;
}
error = -ENOMEM;
insert_inode_hash(root_inode);
@@ -1737,7 +1737,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
sb->s_root = d_make_root(root_inode);
if (!sb->s_root) {
fat_msg(sb, KERN_ERR, "get root inode failed");
- goto out_fail;
+ goto failure_exit;
}
if (sbi->options.discard) {
@@ -1756,11 +1756,13 @@ out_invalid:
if (!silent)
fat_msg(sb, KERN_INFO, "Can't find a valid FAT filesystem");
+failure_exit:
+ iput(fsinfo_inode);
+
+fsinfo_inode_failure:
+ iput(fat_inode);
+
out_fail:
- if (fsinfo_inode)
- iput(fsinfo_inode);
- if (fat_inode)
- iput(fat_inode);
unload_nls(sbi->nls_io);
unload_nls(sbi->nls_disk);
if (sbi->options.iocharset != fat_default_iocharset)
--
2.1.3
SF Markus Elfring <[email protected]> writes:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
On Sat, 29 Nov 2014, OGAWA Hirofumi wrote:
> SF Markus Elfring <[email protected]> writes:
>
> > From: Markus Elfring <[email protected]>
> > 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
Julia Lawall <[email protected]> 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 <[email protected]>
> More labels are more chances of bug (and we don't care micro optimize
> on this error path), isn't it?
I would prefer that a few jump targets can be redirected so that unnecessary
function calls (and corresponding checks) can be avoided.
> Increasing the chance of bugs and bothers developers for analyzer sounds
> like strange.
There are different opinions around source code clarity.
> (And we are initializing those for avoiding to be bothered by choosing
> correct label.
Pointer initialisation is convenient and safe in some use cases, isn't it?
> 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.)
I imagine that a software optimiser can eventually perform better job
if unneeded statements could be omitted, couldn't it?
Regards,
Markus
SF Markus Elfring <[email protected]> writes:
>> More labels are more chances of bug (and we don't care micro optimize
>> on this error path), isn't it?
>
> I would prefer that a few jump targets can be redirected so that unnecessary
> function calls (and corresponding checks) can be avoided.
It is from real bugs in my experience, it saw several times those (mine
and other guys, in linux and others). And I think it doesn't have value
to maintain labels for micro optimization in *this error path* though.
So, if you or analyzer can check bugs by the patches affect to those
label usage in future,
Acked-by: OGAWA Hirofumi <[email protected]>
--
OGAWA Hirofumi <[email protected]>
On Sat, Nov 29, 2014 at 10:59:47PM +0900, OGAWA Hirofumi wrote:
> Julia Lawall <[email protected]> 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
On Mon, 1 Dec 2014, Dan Carpenter wrote:
> On Sat, Nov 29, 2014 at 10:59:47PM +0900, OGAWA Hirofumi wrote:
> > Julia Lawall <[email protected]> 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
> 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
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
>> 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
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
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
I added a paragraph on choosing label names, and updated the example
code to use a better label name. I also cleaned up the example code to
more modern style by moving the allocation out of the initializer and
changing the NULL check.
Perhaps the most common type of error handling bug in the kernel is "one
err bugs". CodingStyle already says that we should "avoid nesting" by
using error labels and one err style error handling tends to have
multiple indent levels, so this was already bad style. But I've added a
new paragraph explaining how to avoid one err bugs by using multiple
error labels which is, hopefully, more clear.
Signed-off-by: Dan Carpenter <[email protected]>
diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 9f28b14..9c8a234 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done. If there is no
cleanup needed then just return directly.
-The rationale is:
+Choose label names which say what the goto does or why the goto exists. An
+example of a good name could be "out_buffer:" if the goto frees "buffer". Avoid
+using GW-BASIC names like "err1:" and "err2:". Also don't name them after the
+goto location like "err_kmalloc_failed:"
+
+The rationale for using gotos is:
- unconditional statements are easier to understand and follow
- nesting is reduced
@@ -403,9 +408,10 @@ The rationale is:
int fun(int a)
{
int result = 0;
- char *buffer = kmalloc(SIZE);
+ char *buffer;
- if (buffer == NULL)
+ buffer = kmalloc(SIZE);
+ if (!buffer)
return -ENOMEM;
if (condition1) {
@@ -413,14 +419,25 @@ int fun(int a)
...
}
result = 1;
- goto out;
+ goto out_buffer;
}
...
-out:
+out_buffer:
kfree(buffer);
return result;
}
+A common type of bug to be aware of it "one err bugs" which look like this:
+
+err:
+ kfree(foo->bar);
+ kfree(foo);
+ return ret;
+
+The bug in this code is that on some exit paths "foo" is NULL. Normally the
+fix for this is to split it up into two error labels "err_bar:" and "err_foo:".
+
+
Chapter 8: Commenting
Comments are good, but there is also a danger of over-commenting. NEVER
> @@ -403,9 +408,10 @@ The rationale is:
> int fun(int a)
> {
> int result = 0;
> - char *buffer = kmalloc(SIZE);
> + char *buffer;
>
> - if (buffer == NULL)
> + buffer = kmalloc(SIZE);
kmalloc actually takes two arguments. Perhaps it would be better to show
something that looks like a valid call.
Otherwise,
Acked-by: Julia Lawall <[email protected]>
julia
> + if (!buffer)
> return -ENOMEM;
>
> if (condition1) {
> @@ -413,14 +419,25 @@ int fun(int a)
> ...
> }
> result = 1;
> - goto out;
> + goto out_buffer;
> }
> ...
> -out:
> +out_buffer:
> kfree(buffer);
> return result;
> }
>
> +A common type of bug to be aware of it "one err bugs" which look like this:
> +
> +err:
> + kfree(foo->bar);
> + kfree(foo);
> + return ret;
> +
> +The bug in this code is that on some exit paths "foo" is NULL. Normally the
> +fix for this is to split it up into two error labels "err_bar:" and "err_foo:".
> +
> +
> Chapter 8: Commenting
>
> Comments are good, but there is also a danger of over-commenting. NEVER
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Tue, 2 Dec 2014 10:09:02 +0100 (CET)
Julia Lawall <[email protected]> wrote:
> kmalloc actually takes two arguments. Perhaps it would be better to show
> something that looks like a valid call.
Agreed; I took the liberty of sticking in a GFP_KERNEL as I applied the
patch with Julia's ack.
Thanks,
jon
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 9f28b14..9c8a234 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits from multiple
> locations and some common work such as cleanup has to be done. If there is no
> cleanup needed then just return directly.
>
> -The rationale is:
> +Choose label names which say what the goto does or why the goto exists. An
> +[...] Avoid
> +using GW-BASIC names like "err1:" and "err2:". Also don't name them after the
> +goto location like "err_kmalloc_failed:"
I find this documentation approach not safe and clear enough so far.
* How should the reference to an other programming language help in the understanding
of the recommended naming convention for jump labels?
* To which source code place should the word "location" refer to?
- jump source
- jump target
Regards,
Markus
On 12/03/14 13:31, SF Markus Elfring wrote:
>> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
>> index 9f28b14..9c8a234 100644
>> --- a/Documentation/CodingStyle
>> +++ b/Documentation/CodingStyle
>> @@ -392,7 +392,12 @@ The goto statement comes in handy when a function exits from multiple
>> locations and some common work such as cleanup has to be done. If there is no
>> cleanup needed then just return directly.
>>
>> -The rationale is:
>> +Choose label names which say what the goto does or why the goto exists. An
>> +[...] Avoid
>> +using GW-BASIC names like "err1:" and "err2:". Also don't name them after the
>> +goto location like "err_kmalloc_failed:"
>
> I find this documentation approach not safe and clear enough so far.
>
> * How should the reference to an other programming language help in the understanding
> of the recommended naming convention for jump labels?
>
> * To which source code place should the word "location" refer to?
> - jump source
> - jump target
I think you digested the paragraph in too small bits. The term "goto
location" looks synonymous to "jump source" to me.
Regards,
Arend
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe backports" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 03, 2014 at 01:31:19PM +0100, SF Markus Elfring wrote:
>
> * To which source code place should the word "location" refer to?
> - jump source
> - jump target
jump target.
regards,
dan carpenter
>> * To which source code place should the word "location" refer to?
>> - jump source
>> - jump target
>
> I think you digested the paragraph in too small bits.
I would prefer to reduce the probability for misunderstandings of the
proposed wording a bit more.
> The term "goto location" looks synonymous to "jump source" to me.
I would interpret it differently because of the specific placement of this key word
before an other term.
Regards,
Markus
On Wed, 3 Dec 2014, Dan Carpenter wrote:
> On Wed, Dec 03, 2014 at 01:31:19PM +0100, SF Markus Elfring wrote:
> >
> > * To which source code place should the word "location" refer to?
> > - jump source
> > - jump target
>
> jump target.
I think you mean source? Or it really is ambiguous. The example was
err_kmalloc_failed, which seems source-related.
julia
>> * To which source code place should the word "location" refer to?
>> - jump source
>> - jump target
>
> jump target.
Thanks for the clarification of your intention.
I wonder then why I got the feedback "That is a useless thing to do."
from you yesterday.
I hope that we can still clarify our different opinions about specific
implementation details in constructive ways.
Regards,
Markus
On Wed, Dec 03, 2014 at 01:52:53PM +0100, Julia Lawall wrote:
> On Wed, 3 Dec 2014, Dan Carpenter wrote:
>
> > On Wed, Dec 03, 2014 at 01:31:19PM +0100, SF Markus Elfring wrote:
> > >
> > > * To which source code place should the word "location" refer to?
> > > - jump source
> > > - jump target
> >
> > jump target.
>
> I think you mean source? Or it really is ambiguous. The example was
> err_kmalloc_failed, which seems source-related.
Yeah. You're right. I misread Markus's email. The goto location is
where the goto is. The label location is where the label is.
regards,
dan carpenter
Sorry. I misread your email. If the code looks like this:
foo = kmalloc();
if (!foo)
goto kmalloc_failed;
The "kmalloc_failed" doesn't add any information. We can see that
kmalloc failed from the context.
regards,
dan carpenter
> Sorry. I misread your email. If the code looks like this:
>
> foo = kmalloc();
> if (!foo)
> goto kmalloc_failed;
>
> The "kmalloc_failed" doesn't add any information.
I find that this such a name approach would fit to your
expectation of a source-oriented labeling of these identifiers.
> We can see that kmalloc failed from the context.
Which name pattern do you find more appropriate in such
an use case?
Regards,
Markus
On 12/03/14 14:24, SF Markus Elfring wrote:
>> Sorry. I misread your email. If the code looks like this:
>>
>> foo = kmalloc();
>> if (!foo)
>> goto kmalloc_failed;
>>
>> The "kmalloc_failed" doesn't add any information.
>
> I find that this such a name approach would fit to your
> expectation of a source-oriented labeling of these identifiers.
>
>
>> We can see that kmalloc failed from the context.
>
> Which name pattern do you find more appropriate in such
> an use case?
I think Dan wants the label to be descriptive about the tasks needed in
the exception handling itself. This makes sense as the exception
handling steps may be reused for different failures in the code.
void foo(void)
{
if (check_a())
goto do_bar;
sub_foo1();
if (checck_b())
goto do_bar;
sub_foo2();
return;
do_bar:
bar();
}
Regards,
Arend
> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe backports" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Which name pattern do you find more appropriate in such
>> an use case?
>
> I think Dan wants the label to be descriptive about the tasks
> needed in the exception handling itself.
I would usually prefer also such a target-oriented labelling
for the affected identifiers.
How are the chances to express an expectation in this direction
unambiguously for the proposed coding style update?
> This makes sense as the exception handling steps may be reused
> for different failures in the code.
I would stress a different reason from my point of view.
Regards,
Markus
On 12/03/14 17:00, SF Markus Elfring wrote:
>>> Which name pattern do you find more appropriate in such
>>> an use case?
>>
>> I think Dan wants the label to be descriptive about the tasks
>> needed in the exception handling itself.
>
> I would usually prefer also such a target-oriented labelling
> for the affected identifiers.
> How are the chances to express an expectation in this direction
> unambiguously for the proposed coding style update?
>
>
>> This makes sense as the exception handling steps may be reused
>> for different failures in the code.
>
> I would stress a different reason from my point of view.
I meant as apposed to using a goto-/source-oriented labelling. Please
provide your point of view. That way the explanations given in this
email exchange might be incorporated in the next round of the proposed
update or at least be used as input.
Regards,
Arend
> Please provide your point of view.
I would like to interpret the key word "goto" from the C programming
language a bit more here so that a better common understanding can
eventually be achieved.
Strong opinions might be floating around for the consistent naming
of jump labels. My reasoning works like the following.
This key word could also be interpreted as two items "go" and "to",
couldn't it?
How much does this variation stress its meaning in a specific direction?
Some software developers would like to express the reason about
an unexpected event at the jump source. But I guess that this approach
increases the risk for a popular story like "goto fail;", doesn't it?
I would prefer not to specify "go to failure".
So I find that there are more variants possible to stress the jump target.
Examples:
* Failure_exit
* out_memory_release
* unregister_item
Regards,
Markus