Re: [PATCH] CodingStyle: Clarify and complete chapter 7

[Mark D Rustad]

Jonathan Corbet  wrote:


On Mon, 25 Jul 2016 14:29:06 +0200
Jean Delvare  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...

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


...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, mrus...@gmail.com


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH] CodingStyle: Clarify and complete chapter 7

[Jonathan Corbet]

On Mon, 25 Jul 2016 14:29:06 +0200
Jean Delvare  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...

>  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

...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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] CodingStyle: Clarify and complete chapter 7

[Jean Delvare]

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.

Signed-off-by: Jean Delvare 
Cc: Markus Elfring 
Cc: Jonathan Corbet 
---
 Documentation/CodingStyle |   25 +++--
 1 file changed, 19 insertions(+), 6 deletions(-)

--- linux-4.7-rc7.orig/Documentation/CodingStyle2016-07-04 
08:01:00.0 +0200
+++ linux-4.7-rc7/Documentation/CodingStyle 2016-07-25 14:25:12.498328631 
+0200
@@ -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


-- 
Jean Delvare
SUSE L3 Support
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html