CodingStyle: Clarify and complete chapter 7
PATCH 1/1 CodingStyle: Clarify and complete chapter 7
Chapter 7 (Centralized exiting of functions) of the coding style documentation is unclear at times, and lacks some information (such as the possibility to indent labels with a single space.) Clarify and complete it.
--- Documentation/CodingStyle | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)
Documentation/CodingStyle +19 -6
@@ -396,9 +396,13 @@ locations and some common work such as c cleanup needed then just return directly. 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:" +example of a good name could be "out_free_buffer:" if the goto frees "buffer". +Avoid using GW-BASIC names like "err1:" and "err2:", as you would have to +renumber them if you ever add or remove exit paths, and they make correctness +difficult to verify anyway. + +It is advised to indent labels with a single space (not tab), so that +"diff -p" does not confuse labels with functions. The rationale for using gotos is:
@@ -425,20 +429,29 @@ The rationale for using gotos is: goto out_buffer; } ... - out_buffer: + out_free_buffer: kfree(buffer); return result; } A common type of bug to be aware of is "one err bugs" which look like this: - err: + 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:". +fix for this is to split it up into two error labels "err_free_bar:" and +"err_free_foo:": + + err_free_bar: + kfree(foo->bar); + err_free_foo: + kfree(foo); + return ret; + +Ideally you should simulate errors to test all exit paths. Chapter 8: Commenting
Signature · 2 lines
Jean Delvare SUSE L3 Support
Jonathan Corbet
maintainerQuoted 4 lines · On Mon, 25 Jul 2016 14:29:06 +0200 Jean Delvare <[email protected]> wrote:
Chapter 7 (Centralized exiting of functions) of the coding style documentation is unclear at times, and lacks some information (such as the possibility to indent labels with a single space.) Clarify and complete it.
OK, I've applied this (finally) to the docs tree, sorry for sitting on it for so long. One question, though...
Documentation/CodingStyle The rationale for using gotos is: · line 444
Quoted 9 lines
A common type of bug to be aware of is "one err bugs" which look like this:
Documentation/CodingStyle +1
-1- err: + err: kfree(foo->bar); kfree(foo); return ret; The bug in this code is that on some exit paths "foo" is NULL. Normally the
...except that kfree() can handle null pointers just fine, so this isn't actually a bug, right? Someday when somebody has time it would be good to come up with a better example.
Thanks,
jon
Quoted 19 lines · Jonathan Corbet
Quoted 4 lines · On Mon, 25 Jul 2016 14:29:06 +0200 Jean Delvare <[email protected]> wrote:
Chapter 7 (Centralized exiting of functions) of the coding style documentation is unclear at times, and lacks some information (such as the possibility to indent labels with a single space.) Clarify and complete it.
OK, I've applied this (finally) to the docs tree, sorry for sitting on it for so long. One question, though...
Quoted 8 lines
A common type of bug to be aware of is "one err bugs" which look like this:
Documentation/CodingStyle +1
-1- err: + err: kfree(foo->bar); kfree(foo); return ret;
Quoted 5 lines
Quoted 1 line
The bug in this code is that on some exit paths "foo" is NULL. Normally the
...except that
kfree()can handle null pointers just fine, so this isn't actually a bug, right? Someday when somebody has time it would be good to come up with a better example.
But if foo is NULL, foo->bar is not NULL and so kfree will have a problem with it. So this is a bug.
--
Mark Rustad, [email protected]
Jonathan Corbet
maintainerQuoted 5 lines · On Sun, 14 Aug 2016 13:12:33 -0700 Mark D Rustad <[email protected]> wrote:
Quoted 5 lines
Quoted 5 lines
Documentation/CodingStyle +1
-0+ err: kfree(foo->bar); kfree(foo); return ret;
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
The bug in this code is that on some exit paths "foo" is NULL. Normally the
...except that
kfree()can handle null pointers just fine, so this isn't actually a bug, right? Someday when somebody has time it would be good to come up with a better example.But if foo is NULL,
foo->baris not NULL and so kfree will have a problem with it. So this is a bug.
Oops, sigh, duh. OK, ignore me. (Though technically kfree will not have a problem with it, since things won't get that far.. :)
jon
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
A common type of bug to be aware of is "one err bugs" which look like this:
Documentation/CodingStyle +1
-1- err: + err: kfree(foo->bar); kfree(foo); return ret;
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
The bug in this code is that on some exit paths "foo" is NULL. Normally the
...except that
kfree()can handle null pointers just fine, so this isn't actually a bug, right? Someday when somebody has time it would be good to come up with a better example.But if foo is NULL,
An important condition …
Quoted 1 line
foo->baris not NULL
I wonder about this information. Which run-time environment will provide this behaviour?
Quoted 1 line
and so kfree will have a problem with it.
I find that the parameter evaluation will result in side effects (because of a null pointer access) which are usually unwanted. So the execution of this function call will eventually not start.
Quoted 1 line
So this is a bug.
How do you think about further software development possibilities to improve corresponding exception handling?
How much can the selection of jump labels influence the software design?
Regards,
Markus