* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
@ 2006-10-29 19:36 ` Matthew Wilcox
2006-10-29 20:11 ` Giangiacomo Mariotti
` (9 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Matthew Wilcox @ 2006-10-29 19:36 UTC (permalink / raw)
To: kernel-janitors
On Sun, Oct 29, 2006 at 11:14:59AM -0800, Giangiacomo Mariotti wrote:
> This little patch cleans the definition of HANDLE_STACK in arch/x86_64/kernel/traps.c because it's syntax is misleading.
Yeah, but you've wrapped it past the 80 column boundary, so you'd need
to fix that. And you didn't put a Signed-off-by on it.
I don't think the outer do ... while is necessary anyway. Is there any
problem with just deleting them?
> #define HANDLE_STACK(cond) \
> - do while (cond) { \
> - unsigned long addr = *stack++; \
> - if (oops_in_progress ? \
> - __kernel_text_address(addr) : \
> - kernel_text_address(addr)) { \
> - /* \
> - * If the address is either in the text segment of the \
> - * kernel, or in the region which contains vmalloc'ed \
> - * memory, it *may* be the address of a calling \
> - * routine; if so, print it so that someone tracing \
> - * down the cause of the crash will be able to figure \
> - * out the call path that was taken. \
> - */ \
> - ops->address(data, addr); \
> + do { \
> + while (cond) { \
> + unsigned long addr = *stack++; \
> + if (oops_in_progress ? \
> + __kernel_text_address(addr) : \
> + kernel_text_address(addr)) { \
> + /* \
> + * If the address is either in the text segment of the \
> + * kernel, or in the region which contains vmalloc'ed \
> + * memory, it *may* be the address of a calling \
> + * routine; if so, print it so that someone tracing \
> + * down the cause of the crash will be able to figure \
> + * out the call path that was taken. \
> + */ \
> + ops->address(data, addr); \
> + } \
> } \
> } while (0)
>
> --
> 1.4.3.2
>
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
2006-10-29 19:36 ` Matthew Wilcox
@ 2006-10-29 20:11 ` Giangiacomo Mariotti
2006-10-29 22:41 ` Giangiacomo Mariotti
` (8 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Giangiacomo Mariotti @ 2006-10-29 20:11 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 199 bytes --]
Description: cleans the definition of HANDLE_STACK in arch/x86_64/kernel/traps.c because it's syntax is misleading.
Signed-off-by: Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com>
[-- Attachment #1.2: Type: text/html, Size: 419 bytes --]
[-- Attachment #2: 0001-cleaning-of-HANDLE_STACK-definition.txt --]
[-- Type: text/plain, Size: 1810 bytes --]
From 2ee917c5863439ed08236855300386511e7380ae Mon Sep 17 00:00:00 2001
From: Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com>
Date: Sun, 29 Oct 2006 12:56:28 +0100
Subject: [PATCH] cleaning of HANDLE_STACK definition
---
arch/x86_64/kernel/traps.c | 30 ++++++++++++++++--------------
1 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/arch/x86_64/kernel/traps.c b/arch/x86_64/kernel/traps.c
index 7819022..a0b1e1f 100644
--- a/arch/x86_64/kernel/traps.c
+++ b/arch/x86_64/kernel/traps.c
@@ -297,20 +297,22 @@ void dump_trace(struct task_struct *tsk,
* iteration will eventually trigger.
*/
#define HANDLE_STACK(cond) \
- do while (cond) { \
- unsigned long addr = *stack++; \
- if (oops_in_progress ? \
- __kernel_text_address(addr) : \
- kernel_text_address(addr)) { \
- /* \
- * If the address is either in the text segment of the \
- * kernel, or in the region which contains vmalloc'ed \
- * memory, it *may* be the address of a calling \
- * routine; if so, print it so that someone tracing \
- * down the cause of the crash will be able to figure \
- * out the call path that was taken. \
- */ \
- ops->address(data, addr); \
+ do { \
+ while (cond) { \
+ unsigned long addr = *stack++; \
+ if (oops_in_progress ? \
+ __kernel_text_address(addr) : \
+ kernel_text_address(addr)) { \
+ /* \
+ * If the address is either in the text segment of the \
+ * kernel, or in the region which contains vmalloc'ed \
+ * memory, it *may* be the address of a calling \
+ * routine; if so, print it so that someone tracing \
+ * down the cause of the crash will be able to figure \
+ * out the call path that was taken. \
+ */ \
+ ops->address(data, addr); \
+ } \
} \
} while (0)
--
1.4.3.2
[-- Attachment #3: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply related [flat|nested] 12+ messages in thread* [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
2006-10-29 19:36 ` Matthew Wilcox
2006-10-29 20:11 ` Giangiacomo Mariotti
@ 2006-10-29 22:41 ` Giangiacomo Mariotti
2006-10-30 3:12 ` Amit Choudhary
` (7 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Giangiacomo Mariotti @ 2006-10-29 22:41 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 2923 bytes --]
Matthew Wilcox wrote :
>you've wrapped it past the 80 column boundary, so you'd need to fix that......
>you didn't put a signed-off-by on it.
This one is my previous patch modified.
>I don't think the outer do ..... while is necessary anyway.Is there any problem
>with just deleting them?
I thought about it,but then I tried to change this misleading code without being too
destructive,I thought that the previous author could have had a good reason to
use the do{}while(0).
Description: clean the definition of HANDLE_STACK in arch/x86_64/kernel/traps.c
because it's syntax is misleading.
Signed-off-by: Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com>
-------
From cadb09c6ef0cf2874b1ed3813ea32848cb8c479c Mon Sep 17 00:00:00 2001
From: Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com>
Date: Sun, 29 Oct 2006 16:22:03 +0100
Subject: [PATCH] cleaning of HANDLE_STACK definition
---
arch/x86_64/kernel/traps.c | 32 ++++++++++++++++++--------------
1 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/arch/x86_64/kernel/traps.c b/arch/x86_64/kernel/traps.c
index 7819022..5573a21 100644
--- a/arch/x86_64/kernel/traps.c
+++ b/arch/x86_64/kernel/traps.c
@@ -297,20 +297,24 @@ void dump_trace(struct task_struct *tsk,
* iteration will eventually trigger.
*/
#define HANDLE_STACK(cond) \
- do while (cond) { \
- unsigned long addr = *stack++; \
- if (oops_in_progress ? \
- __kernel_text_address(addr) : \
- kernel_text_address(addr)) { \
- /* \
- * If the address is either in the text segment of the \
- * kernel, or in the region which contains vmalloc'ed \
- * memory, it *may* be the address of a calling \
- * routine; if so, print it so that someone tracing \
- * down the cause of the crash will be able to figure \
- * out the call path that was taken. \
- */ \
- ops->address(data, addr); \
+ do { \
+ while (cond) { \
+ unsigned long addr = *stack++; \
+ if (oops_in_progress ? \
+ __kernel_text_address(addr) : \
+ kernel_text_address(addr)) { \
+ /* \
+ * If the address is either in the text \
+ * segment of the kernel, or in the region \
+ * which contains vmalloc'ed memory, it may \
+ * be the address of a calling routine; if \
+ * so, print it so that someone tracing \
+ * down the cause of the crash will be able \
+ * to figure out the call path that \
+ * was taken. \
+ */ \
+ ops->address(data, addr); \
+ } \
} \
} while (0)
--
1.4.3.2
[-- Attachment #1.2: Type: text/html, Size: 5186 bytes --]
[-- Attachment #2: 0001-cleaning-of-HANDLE_STACK-definition.txt --]
[-- Type: text/plain, Size: 1821 bytes --]
From cadb09c6ef0cf2874b1ed3813ea32848cb8c479c Mon Sep 17 00:00:00 2001
From: Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com>
Date: Sun, 29 Oct 2006 16:22:03 +0100
Subject: [PATCH] cleaning of HANDLE_STACK definition
---
arch/x86_64/kernel/traps.c | 32 ++++++++++++++++++--------------
1 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/arch/x86_64/kernel/traps.c b/arch/x86_64/kernel/traps.c
index 7819022..5573a21 100644
--- a/arch/x86_64/kernel/traps.c
+++ b/arch/x86_64/kernel/traps.c
@@ -297,20 +297,24 @@ void dump_trace(struct task_struct *tsk,
* iteration will eventually trigger.
*/
#define HANDLE_STACK(cond) \
- do while (cond) { \
- unsigned long addr = *stack++; \
- if (oops_in_progress ? \
- __kernel_text_address(addr) : \
- kernel_text_address(addr)) { \
- /* \
- * If the address is either in the text segment of the \
- * kernel, or in the region which contains vmalloc'ed \
- * memory, it *may* be the address of a calling \
- * routine; if so, print it so that someone tracing \
- * down the cause of the crash will be able to figure \
- * out the call path that was taken. \
- */ \
- ops->address(data, addr); \
+ do { \
+ while (cond) { \
+ unsigned long addr = *stack++; \
+ if (oops_in_progress ? \
+ __kernel_text_address(addr) : \
+ kernel_text_address(addr)) { \
+ /* \
+ * If the address is either in the text \
+ * segment of the kernel, or in the region \
+ * which contains vmalloc'ed memory, it may \
+ * be the address of a calling routine; if \
+ * so, print it so that someone tracing \
+ * down the cause of the crash will be able \
+ * to figure out the call path that \
+ * was taken. \
+ */ \
+ ops->address(data, addr); \
+ } \
} \
} while (0)
--
1.4.3.2
[-- Attachment #3: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (2 preceding siblings ...)
2006-10-29 22:41 ` Giangiacomo Mariotti
@ 2006-10-30 3:12 ` Amit Choudhary
2006-10-30 12:07 ` Darren Jenkins
` (6 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Amit Choudhary @ 2006-10-30 3:12 UTC (permalink / raw)
To: kernel-janitors
--- Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com> wrote:
> >I don't think the outer do ..... while is necessary anyway.Is there any problem
> >with just deleting them?
> I thought about it,but then I tried to change this misleading code without being too
> destructive,I thought that the previous author could have had a good reason to
> use the do{}while(0).
>
There are couple of good reasons for using a do - while block in a macro having multiple
statements. Chapter 11 of kernel coding style recommends using the do - while block. A macro is
generally used as:
MACRO(X);
The semicolon will be handled correctly if the macro is enclosed in a do - while block.
----
Chapter 11: Macros, Enums and RTL
Macros with multiple statements should be enclosed in a do - while block:
#define macrofun(a, b, c) \
do { \
if (a = 5) \
do_this(b, c); \
} while (0)
----
Regards,
Amit
____________________________________________________________________________________
Want to start your own business? Learn how on Yahoo! Small Business
(http://smallbusiness.yahoo.com)
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (3 preceding siblings ...)
2006-10-30 3:12 ` Amit Choudhary
@ 2006-10-30 12:07 ` Darren Jenkins
2006-10-30 12:22 ` Giangiacomo Mariotti
` (5 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Darren Jenkins @ 2006-10-30 12:07 UTC (permalink / raw)
To: kernel-janitors
G'day all
On 10/30/06, Amit Choudhary <amit2030@yahoo.com> wrote:
>
> There are couple of good reasons for using a do - while block in a macro having multiple
> statements.
While you are spot on here and that explains what the code did before
the patch, I think the point that Matthew Wilcox was getting at is
that _after_ this patch the "do { } while (0)", now apears to wrap a
_single_ "while (cond) { }" statement, making it surperflous.
Darren J.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (4 preceding siblings ...)
2006-10-30 12:07 ` Darren Jenkins
@ 2006-10-30 12:22 ` Giangiacomo Mariotti
2006-10-30 12:52 ` Jaco Kroon
` (4 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Giangiacomo Mariotti @ 2006-10-30 12:22 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 496 bytes --]
Darren Jenkins wrote:
>While you are spot on here and that explains what the code did before
>the patch, I think the point that Matthew Wilcox was getting at is
>that _after_ this patch the "do { } while (0)", now apears to wrap a
>_single_ "while (cond) { }" statement, making it surperflous.
Yes,this was my point too,but then I tryied to maintain the general style of the code fragment,
so now the code is conformant to the kernel code style and it's clearer than before.
[-- Attachment #1.2: Type: text/html, Size: 918 bytes --]
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (5 preceding siblings ...)
2006-10-30 12:22 ` Giangiacomo Mariotti
@ 2006-10-30 12:52 ` Jaco Kroon
2006-10-30 13:26 ` Giangiacomo Mariotti
` (3 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Jaco Kroon @ 2006-10-30 12:52 UTC (permalink / raw)
To: kernel-janitors
> Darren Jenkins wrote:
>>While you are spot on here and that explains what the code did before
>>the patch, I think the point that Matthew Wilcox was getting at is
>>that _after_ this patch the "do { } while (0)", now apears to wrap a
>>_single_ "while (cond) { }" statement, making it surperflous.
> Yes,this was my point too,but then I tryied to maintain the general style
> of the code fragment,
> so now the code is conformant to the kernel code style and it's clearer
> than before.
As already explained, the do { } while(0) is actually there for a very
good reason.
Consider a fictitious macro called m() that looks something like:
#define m() while(foo) { blah() }
Now, if I use that macro in the following construct I've got a problem:
if (bleh)
m();
else
bar();
That won't actually compile since it expands to:
if (bleh)
while(foo) { blah() };
else
bar();
Now, that semicolon at the directly after the } is what causes the
problem. So wrapping it up in a do {} while(0) solves that problem:
if (bleh)
do {
while(foo) { blah() }
while(0);
else
bar();
Does actually work.
My 5c worth,
Jaco
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (6 preceding siblings ...)
2006-10-30 12:52 ` Jaco Kroon
@ 2006-10-30 13:26 ` Giangiacomo Mariotti
2006-10-30 13:42 ` Jaco Kroon
` (2 subsequent siblings)
10 siblings, 0 replies; 12+ messages in thread
From: Giangiacomo Mariotti @ 2006-10-30 13:26 UTC (permalink / raw)
To: kernel-janitors
Jaco Kroon <jaco@kroon.co.za> wrote:
>As already explained, the do { } while(0) is actually there for a very
>good reason.
Maybe I did not explain clearly what I meant with my previous e-mails.
I know that the do{}while(0) syntax is the right one for macros(and I do know why), but
if you look at the code in arch/x86_64/kernel/traps.c, HANDLE_STACK does not seem to
need this kind of syntax,by looking at how it is used;anyway,to be sure about that,
I should have checked the entire file and I could have created a bug where it wasn't before.
What I wanted to do was simply to clear the bad syntax of HANDLE_STACK definition,
and using the general syntax of do{}while(0) was the better way to do that trying to
be conservative.
Giangiacomo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (7 preceding siblings ...)
2006-10-30 13:26 ` Giangiacomo Mariotti
@ 2006-10-30 13:42 ` Jaco Kroon
2006-10-30 13:46 ` Giangiacomo Mariotti
2006-10-30 13:52 ` Giangiacomo Mariotti
10 siblings, 0 replies; 12+ messages in thread
From: Jaco Kroon @ 2006-10-30 13:42 UTC (permalink / raw)
To: kernel-janitors
> Jaco Kroon <jaco@kroon.co.za> wrote:
>>As already explained, the do { } while(0) is actually there for a very
>>good reason.
> Maybe I did not explain clearly what I meant with my previous e-mails.
> I know that the do{}while(0) syntax is the right one for macros(and I do
> know why), but
> if you look at the code in arch/x86_64/kernel/traps.c, HANDLE_STACK does
> not seem to
> need this kind of syntax,by looking at how it is used;anyway,to be sure
> about that,
> I should have checked the entire file and I could have created a bug where
> it wasn't before.
> What I wanted to do was simply to clear the bad syntax of HANDLE_STACK
> definition,
> and using the general syntax of do{}while(0) was the better way to do that
> trying to
> be conservative.
Either way. IMHO it's always a good idea to use those do {} while(0)
wrappers even though they are not needed for current usage. Remember that
from code a macro call looks exactly like a function call, as such I would
reckon it's best to try and keep them looking like that, so imho it should
always be there. It's safer, and is more future proof than without them.
Jaco
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (8 preceding siblings ...)
2006-10-30 13:42 ` Jaco Kroon
@ 2006-10-30 13:46 ` Giangiacomo Mariotti
2006-10-30 13:52 ` Giangiacomo Mariotti
10 siblings, 0 replies; 12+ messages in thread
From: Giangiacomo Mariotti @ 2006-10-30 13:46 UTC (permalink / raw)
To: kernel-janitors
>Jaco Kroon <jaco@kroon.co.za> wrote:
>>As already explained, the do { } while(0) is actually there for a very
>>good reason.
>Maybe I did not explain clearly what I meant with my previous e-mails.
>I know that the do{}while(0) syntax is the right one for macros(and I do know why), but
>if you look at the code in arch/x86_64/kernel/traps.c, HANDLE_STACK does not seem to
>need this kind of syntax,by looking at how it is used;anyway,to be sure about that,
>I should have checked the entire file and I could have created a bug where it wasn't before.
>What I wanted to do was simply to clear the bad syntax of HANDLE_STACK definition,
>and using the general syntax of do{}while(0) was the better way to do that trying to
>be conservative.
>
>Giangiacomo
well,not the entire file,just a function,anyway.....
Giangiacomo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition
2006-10-29 19:14 [KJ] [PATCH] cleaning of HANDLE_STACK definition Giangiacomo Mariotti
` (9 preceding siblings ...)
2006-10-30 13:46 ` Giangiacomo Mariotti
@ 2006-10-30 13:52 ` Giangiacomo Mariotti
10 siblings, 0 replies; 12+ messages in thread
From: Giangiacomo Mariotti @ 2006-10-30 13:52 UTC (permalink / raw)
To: kernel-janitors
Jaco Kroon <jaco@kroon.co.za> wrote:
>Either way. IMHO it's always a good idea to use those do {} while(0)
>wrappers even though they are not needed for current usage. Remember that
>from code a macro call looks exactly like a function call, as such I would
>reckon it's best to try and keep them looking like that, so imho it should
>always be there. It's safer, and is more future proof than without them.
Yes,I agree with you,and if you look at my patch
you'll see that I used the do{}while(0) syntax.
Giangiacomo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 12+ messages in thread