On Tue, Jan 06, 2015 at 05:53:39PM +0100, Sam Ravnborg wrote:

On Tue, Jan 06, 2015 at 05:44:56PM +0200, Michael S. Tsirkin wrote:

quoted

Macros within arch/sparc/include/asm/uaccess_64.h are made harder to
read because they violate a bunch of coding style rules.

Fix it up.

As per Davem's earlier mail please prefix using sparc32/sparc64.

I did put in uaccess_64 - insufficient?

quoted

-#define __put_user_nocheck(data,addr,size) ({ \
-register int __pu_ret; \
-switch (size) { \
-case 1: __put_user_asm(data,b,addr,__pu_ret); break; \
-case 2: __put_user_asm(data,h,addr,__pu_ret); break; \
-case 4: __put_user_asm(data,w,addr,__pu_ret); break; \
-case 8: __put_user_asm(data,x,addr,__pu_ret); break; \
-default: __pu_ret = __put_user_bad(); break; \
-} __pu_ret; })
-
-#define __put_user_asm(x,size,addr,ret)					\
+#define __put_user_nocheck(data, addr, size) ({ \
+	register int __pu_ret; \
+	switch (size) { \
+	case 1: \
+		__put_user_asm(data, b, addr, __pu_ret); \
+		break; \
+	case 2: \
+		__put_user_asm(data, h, addr, __pu_ret); \
+		break; \
+	case 4: \
+		__put_user_asm(data, w, addr, __pu_ret); \
+		break; \
+	case 8: \
+		__put_user_asm(data, x, addr, __pu_ret); \
+		break; \
+	default: \
+		__pu_ret = __put_user_bad(); \
+		break; \
+	} \
+	__pu_ret; \
+})

No matter what coding style says - the above is much less readable than the
original version.

I guess you approve the rest of the changes then?


I get it you like it that 
	case 1: __get_user_asm(__gu_val,ub,addr,__gu_ret); break;
has the whole case on the same line?
Is that the issue?

quoted

-#define __get_user_nocheck(data,addr,size,type) ({ \
-register int __gu_ret; \
-register unsigned long __gu_val; \
-switch (size) { \
-case 1: __get_user_asm(__gu_val,ub,addr,__gu_ret); break; \
-case 2: __get_user_asm(__gu_val,uh,addr,__gu_ret); break; \
-case 4: __get_user_asm(__gu_val,uw,addr,__gu_ret); break; \
-case 8: __get_user_asm(__gu_val,x,addr,__gu_ret); break; \
-default: __gu_val = 0; __gu_ret = __get_user_bad(); break; \
-} data = (__force type) __gu_val; __gu_ret; })
-
-#define __get_user_nocheck_ret(data,addr,size,type,retval) ({ \
-register unsigned long __gu_val __asm__ ("l1"); \
-switch (size) { \
-case 1: __get_user_asm_ret(__gu_val,ub,addr,retval); break; \
-case 2: __get_user_asm_ret(__gu_val,uh,addr,retval); break; \
-case 4: __get_user_asm_ret(__gu_val,uw,addr,retval); break; \
-case 8: __get_user_asm_ret(__gu_val,x,addr,retval); break; \
-default: if (__get_user_bad()) return retval; \
-} data = (__force type) __gu_val; })
-
-#define __get_user_asm(x,size,addr,ret)					\
+#define __get_user_nocheck(data, addr, size, type) ({ \
+	register int __gu_ret; \
+	register unsigned long __gu_val; \
+	switch (size) { \
+		case 1: \
+			__get_user_asm(__gu_val, ub, addr, __gu_ret); \
+			break; \
+		case 2: \
+			__get_user_asm(__gu_val, uh, addr, __gu_ret); \
+			break; \
+		case 4: \
+			__get_user_asm(__gu_val, uw, addr, __gu_ret); \
+			break; \
+		case 8: \
+			__get_user_asm(__gu_val, x, addr, __gu_ret); \
+			break; \
+		default: \
+			__gu_val = 0; \
+			__gu_ret = __get_user_bad(); \
+			break; \
+	} data = (__force type) __gu_val; __gu_ret; \
+})
+
+#define __get_user_nocheck_ret(data, addr, size, type, retval) ({ \
+	register unsigned long __gu_val __asm__ ("l1"); \
+	switch (size) { \
+	case 1: \
+		__get_user_asm_ret(__gu_val, ub, addr, retval); \
+		break; \
+	case 2: \
+		__get_user_asm_ret(__gu_val, uh, addr, retval); \
+		break; \
+	case 4: \
+		__get_user_asm_ret(__gu_val, uw, addr, retval); \
+		break; \
+	case 8: \
+		__get_user_asm_ret(__gu_val, x, addr, retval); \
+		break; \
+	default: \
+		if (__get_user_bad()) \
+			return retval; \
+	} \
+	data = (__force type) __gu_val; \
+})
+

Same comment for this code chunk.

	Sam

Well I donnu.  When I had to fix bugs there, it was pretty confusing to
me, conding style is no a holy book but it's there for a reason.

Lack of spaces after comma makes it so much harder
to count parameters.

Also:

quoted

-default: if (__get_user_bad()) return retval; \
-} data = (__force type) __gu_val; })

return on same line with if and code after the closing {}
makes it look confusingly like the more conventional:

 if (__get_user_bad())
 	data = (__force type) __gu_val;

-- 
MST