Re: [PATCH 1/2] Massive code cleanup of sys_msync()

[Christoph Hellwig]

On Tue, Jan 15, 2008 at 07:02:44PM +0300, Anton Salikhmetov wrote:
> +++ b/mm/msync.c
> @@ -1,24 +1,25 @@
>  /*
>   *	linux/mm/msync.c
>   *
> + * The msync() system call.
>   * Copyright (C) 1994-1999  Linus Torvalds
> + *
> + * Massive code cleanup.
> + * Copyright (C) 2008 Anton Salikhmetov <salikhmetov@xxxxxxxxx>

Please don't put the changelog in here, that's what the log in the SCM
is for.  And while you're at it remove the confusing file name comment.
This should now look something like:

/*
 * The msync() system call.
 *
 * Copyright (C) 1994-1999  Linus Torvalds
 * Copyright (C) 2008 Anton Salikhmetov <salikhmetov@xxxxxxxxx>
 */

> @@ -33,71 +34,65 @@ asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
>  	unsigned long end;
>  	struct mm_struct *mm = current->mm;
>  	struct vm_area_struct *vma;
> -	int unmapped_error = 0;
> -	int error = -EINVAL;
> +	int error = 0, unmapped_error = 0;
>  
>  	if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
> -		goto out;
> +		return -EINVAL;
>  	if (start & ~PAGE_MASK)
> -		goto out;
> +		return -EINVAL;

The goto out for a simple return style is used quite commonly in kernel
code to have a single return statement which makes code maintaince, e.g.
adding locks or allocations simpler.  Not sure that getting rid of it
makes a lot of sense.

> +		file = vma->vm_file;
> +		if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {

Given that file is assigned just above it would be more readable to test
it first.

> +			if (error)
> +				return error;

This should be an goto out, returns out of the middle of the function
make reading and maintaining the code not so nice.

>  	return error ? : unmapped_error;

Care to get rid of this odd GNU extension while you're at it and use
the proper

	return error ? error : unmapped_error;

?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/