[KJ] [PATCH] cleaning of HANDLE_STACK definition

[KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

[-- Attachment #1.1: Type: text/plain, Size: 134 bytes --]

This little patch cleans the definition of HANDLE_STACK in arch/x86_64/kernel/traps.c because it's syntax is misleading.







[-- Attachment #1.2: Type: text/html, Size: 346 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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Matthew Wilcox]

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

[KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

[-- 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

[KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

[-- 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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Amit Choudhary]

--- 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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Darren Jenkins]

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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

[-- 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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Jaco Kroon]

> 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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Jaco Kroon]

> 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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

>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

Re: [KJ] [PATCH] cleaning of HANDLE_STACK definition

[Giangiacomo Mariotti]

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