| 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
| ||
|
Message-ID: <20060727162434.GA29489@elte.hu>
Date: Thu, 27 Jul 2006 18:24:34 +0200
From: Ingo Molnar <mingo@...e.hu>
To: Alexey Dobriyan <adobriyan@...il.com>
Cc: Andrew Morton <akpm@...l.org>, linux-kernel@...r.kernel.org
Subject: Re: [patch] ipc/msg.c: clean up coding style
* Alexey Dobriyan <adobriyan@...il.com> wrote:
> On Thu, Jul 27, 2006 at 03:53:21PM +0200, Ingo Molnar wrote:
> > clean up ipc/msg.c to conform to Documentation/CodingStyle. (before it
> > was an inconsistent hodepodge of various coding styles)
>
> > --- linux.orig/ipc/msg.c
> > +++ linux/ipc/msg.c
> > -/* one msg_receiver structure for each sleeping receiver */
> > +/*
> > + * one msg_receiver structure for each sleeping receiver:
> > + */
>
> Was OK.
but it was not consistent. So i made it so.
> > struct msg_receiver {
> > - struct list_head r_list;
> > - struct task_struct* r_tsk;
> > + struct list_head r_list;
> > + struct task_struct *r_tsk;
>
> Moving * to name is OK, but lining up all names is probably not.
again, it was not consistent, i made it so. It's perfectly fine to line
up names, especially in structure declarations.
> > - int r_mode;
> > - long r_msgtype;
> > - long r_maxsize;
> > + int r_mode;
> > + long r_msgtype;
> > + long r_maxsize;
> >
> > - struct msg_msg* volatile r_msg;
> > + volatile struct msg_msg *r_msg;
>
> First, it was a volatile pointer, now pointer points to volatile data.
> Right?
the volatile is quite likely bogus anyway. It made no difference to the
assembly output.
> > /* one msg_sender for each sleeping sender */
> > struct msg_sender {
> > - struct list_head list;
> > - struct task_struct* tsk;
> > + struct list_head list;
> > + struct task_struct *tsk;
>
> Let's not lineup fields.
again, consistency.
> > -static atomic_t msg_bytes = ATOMIC_INIT(0);
> > -static atomic_t msg_hdrs = ATOMIC_INIT(0);
> > +static atomic_t msg_bytes = ATOMIC_INIT(0);
> > +static atomic_t msg_hdrs = ATOMIC_INIT(0);
>
> Was OK.
again, consistency.
> > -asmlinkage long sys_msgget (key_t key, int msgflg)
> > +asmlinkage long sys_msgget(key_t key, int msgflg)
> > {
> > - int id, ret = -EPERM;
> > struct msg_queue *msq;
> > + int id, ret = -EPERM;
>
> For what?
that's how i mark functions that i cleaned up, i order the variable
lines by length. (take a look at kernel/sched.c, kernel/rtmutex.c,
kernel/lockdep.c, etc., etc.) It also looks a tiny bit more structured
than the usual random dump of variable definitions.
> > -static inline unsigned long copy_msqid_to_user(void __user *buf, struct msqid64_ds *in, int version)
> > +static inline unsigned long
> > +copy_msqid_to_user(void __user *buf, struct msqid64_ds *in, int version)
>
> Let's not go BSD way.
lets not go for longer than 80 chars.
> > case IPC_OLD:
> > - {
> > + {
>
> Or
> case IPC_OLD: {
again, consistency. Most places in this file used the one to what i
corrected it above.
> > -static inline unsigned long copy_msqid_from_user(struct msq_setbuf *out, void __user *buf, int version)
> > +static inline unsigned long
> > +copy_msqid_from_user(struct msq_setbuf *out, void __user *buf, int version)
>
> Let's not go BSD way.
again, lets not have overlong line 80 prototypes.
> > - int err, version;
> > - struct msg_queue *msq;
> > - struct msq_setbuf setbuf;
> > struct kern_ipc_perm *ipcp;
> > -
> > + struct msq_setbuf setbuf;
> > + struct msg_queue *msq;
> > + int err, version;
>
> There must be logic about moving up and down, but I failed to extract
> it. Except you want to see err, rv, retval, ... last.
take a look at the resulting file.
> > - msg = (struct msg_msg*) msr_d.r_msg;
> > + msg = (struct msg_msg*)msr_d.r_msg;
>
> msg_msg *, while you're at it.
yep.
> > @@ -827,20 +848,20 @@ static int sysvipc_msg_proc_show(struct
> > struct msg_queue *msq = it;
> >
> > return seq_printf(s,
> > - "%10d %10d %4o %10lu %10lu %5u %5u %5u %5u %5u %5u %10lu %10lu %10lu\n",
> > - msq->q_perm.key,
> > - msq->q_id,
> > - msq->q_perm.mode,
> > - msq->q_cbytes,
> > - msq->q_qnum,
> > - msq->q_lspid,
> > - msq->q_lrpid,
> > - msq->q_perm.uid,
> > - msq->q_perm.gid,
> > - msq->q_perm.cuid,
> > - msq->q_perm.cgid,
> > - msq->q_stime,
> > - msq->q_rtime,
> > - msq->q_ctime);
> > + "%10d %10d %4o %10lu %10lu %5u %5u %5u %5u %5u %5u %10lu %10lu %10lu\n",
> > + msq->q_perm.key,
> > + msq->q_id,
> > + msq->q_perm.mode,
> > + msq->q_cbytes,
> > + msq->q_qnum,
> > + msq->q_lspid,
> > + msq->q_lrpid,
> > + msq->q_perm.uid,
> > + msq->q_perm.gid,
> > + msq->q_perm.cuid,
> > + msq->q_perm.cgid,
> > + msq->q_stime,
> > + msq->q_rtime,
> > + msq->q_ctime);
>
> Was OK.
stupid 2 spaces for every line was not OK but we/you can reintroduce
them after this patch goes in.
(anyway, i'd suggest to also spend some of your review energy on all the
other 1000 zillion files that have very real and obvious style problems,
instead of redundantly finetuning the ones i did? The last thing we need
is the needless blocking of obviously right cleanup patches on small
silly details, which patches already vastly improve what was there
before. We can quibble about BSD or non-BSD prototypes after all that
has been finished.)
Ingo
-
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