| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 6 Jan 2015 19:19:02 +0200
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Sam Ravnborg <sam@...nborg.org>
Cc: linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
linux-arch@...r.kernel.org,
"David S. Miller" <davem@...emloft.net>, sparclinux@...r.kernel.org
Subject: Re: [PATCH v2 26/40] arch/sparc: uaccess_64 macro whitespace fixes
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:
> > 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?
> > -#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?
>
> > -#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:
> > -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists