linux-kernel - Re: [PATCH 1/2] Massive code cleanup of sys

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080115175705.GA21557@infradead.org>
Date:	Tue, 15 Jan 2008 17:57:05 +0000
From:	Christoph Hellwig <hch@...radead.org>
To:	Anton Salikhmetov <salikhmetov@...il.com>
Cc:	linux-mm@...ck.org, jakob@...hought.net,
	linux-kernel@...r.kernel.org, valdis.kletnieks@...edu,
	riel@...hat.com, ksm@...dk, staubach@...hat.com,
	jesper.juhl@...il.com, torvalds@...ux-foundation.org,
	a.p.zijlstra@...llo.nl, akpm@...ux-foundation.org,
	protasnb@...il.com, miklos@...redi.hu
Subject: Re: [PATCH 1/2] Massive code cleanup of sys_msync()

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@...il.com>

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@...il.com>
 */

> @@ -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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/