Re: [PATCH 5/4] ipc,shm: minor cleanups

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

On 04/23/2014 04:53 AM, Davidlohr Bueso wrote:
> -  Breakup long function names/args.
> -  Cleaup variable declaration.
> -  s/current->mm/mm
> 
> Signed-off-by: Davidlohr Bueso <davidlohr@hp.com>
> ---
>  ipc/shm.c | 40 +++++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 23 deletions(-)
> 
> diff --git a/ipc/shm.c b/ipc/shm.c
> index f000696..584d02e 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -480,15 +480,13 @@ static const struct vm_operations_struct shm_vm_ops = {
>  static int newseg(struct ipc_namespace *ns, struct ipc_params *params)
>  {
>  	key_t key = params->key;
> -	int shmflg = params->flg;
> +	int id, error, shmflg = params->flg;

It's largely a matter of taste (and I may be in a minority), and I know
there's certainly precedent in the kernel code, but I don't much like the 
style of mixing variable declarations that have initializers, with other
unrelated declarations (e.g., variables without initializers). What is 
the gain? One less line of text? That's (IMO) more than offset by the 
small loss of readability.

Cheers,

Michael

>  	size_t size = params->u.size;
> -	int error;
> -	struct shmid_kernel *shp;
>  	size_t numpages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> -	struct file *file;
>  	char name[13];
> -	int id;
>  	vm_flags_t acctflag = 0;
> +	struct shmid_kernel *shp;
> +	struct file *file;
>  
>  	if (size < SHMMIN || size > ns->shm_ctlmax)
>  		return -EINVAL;
> @@ -681,7 +679,8 @@ copy_shmid_from_user(struct shmid64_ds *out, void __user *buf, int version)
>  	}
>  }
>  
> -static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminfo64 *in, int version)
> +static inline unsigned long copy_shminfo_to_user(void __user *buf,
> +						 struct shminfo64 *in, int version)
>  {
>  	switch (version) {
>  	case IPC_64:
> @@ -711,8 +710,8 @@ static inline unsigned long copy_shminfo_to_user(void __user *buf, struct shminf
>   * Calculate and add used RSS and swap pages of a shm.
>   * Called with shm_ids.rwsem held as a reader
>   */
> -static void shm_add_rss_swap(struct shmid_kernel *shp,
> -	unsigned long *rss_add, unsigned long *swp_add)
> +static void shm_add_rss_swap(struct shmid_kernel *shp, unsigned long *rss_add,
> +			     unsigned long *swp_add)
>  {
>  	struct inode *inode;
>  
> @@ -739,7 +738,7 @@ static void shm_add_rss_swap(struct shmid_kernel *shp,
>   * Called with shm_ids.rwsem held as a reader
>   */
>  static void shm_get_stat(struct ipc_namespace *ns, unsigned long *rss,
> -		unsigned long *swp)
> +			 unsigned long *swp)
>  {
>  	int next_id;
>  	int total, in_use;
> @@ -1047,21 +1046,16 @@ out_unlock1:
>  long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	      unsigned long shmlba)
>  {
> -	struct shmid_kernel *shp;
> -	unsigned long addr;
> -	unsigned long size;
> +	unsigned long addr, size, flags, prot, populate = 0;
>  	struct file *file;
> -	int    err;
> -	unsigned long flags;
> -	unsigned long prot;
> -	int acc_mode;
> +	int acc_mode, err = -EINVAL;
>  	struct ipc_namespace *ns;
>  	struct shm_file_data *sfd;
> +	struct shmid_kernel *shp;
>  	struct path path;
>  	fmode_t f_mode;
> -	unsigned long populate = 0;
> +	struct mm_struct *mm = current->mm;
>  
> -	err = -EINVAL;
>  	if (shmid < 0)
>  		goto out;
>  	else if ((addr = (ulong)shmaddr)) {
> @@ -1161,20 +1155,20 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	if (err)
>  		goto out_fput;
>  
> -	down_write(&current->mm->mmap_sem);
> +	down_write(&mm->mmap_sem);
>  	if (addr && !(shmflg & SHM_REMAP)) {
>  		err = -EINVAL;
>  		if (addr + size < addr)
>  			goto invalid;
>  
> -		if (find_vma_intersection(current->mm, addr, addr + size))
> +		if (find_vma_intersection(mm, addr, addr + size))
>  			goto invalid;
>  		/*
>  		 * If shm segment goes below stack, make sure there is some
>  		 * space left for the stack to grow (at least 4 pages).
>  		 */
> -		if (addr < current->mm->start_stack &&
> -		    addr > current->mm->start_stack - size - PAGE_SIZE * 5)
> +		if (addr < mm->start_stack &&
> +		    addr > mm->start_stack - size - PAGE_SIZE * 5)
>  			goto invalid;
>  	}
>  
> @@ -1184,7 +1178,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
>  	if (IS_ERR_VALUE(addr))
>  		err = (long)addr;
>  invalid:
> -	up_write(&current->mm->mmap_sem);
> +	up_write(&mm->mmap_sem);
>  	if (populate)
>  		mm_populate(addr, populate);
>  
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/