Re: [patch] ipc/msg.c: clean up coding style

[Ingo Molnar <mingo@elte.hu>]

* Alexey Dobriyan <adobriyan@gmail.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