From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH 10/14] kvm tools: Rename raw_image_ops to blk_dev_ops
Date: Wed, 18 May 2011 10:51:55 +0200
Message-ID: <20110518085155.GI14805@elte.hu>
References: <1305706755-2816-1-git-send-email-asias.hejun@gmail.com>
 <1305706755-2816-10-git-send-email-asias.hejun@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Pekka Enberg <penberg@kernel.org>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	Sasha Levin <levinsasha928@gmail.com>,
	Prasad Joshi <prasadjoshi124@gmail.com>, kvm@vger.kernel.org
To: Asias He <asias.hejun@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx3.mail.elte.hu ([157.181.1.138]:58215 "EHLO mx3.mail.elte.hu"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751573Ab1ERIwF (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 18 May 2011 04:52:05 -0400
Content-Disposition: inline
In-Reply-To: <1305706755-2816-10-git-send-email-asias.hejun@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


* Asias He <asias.hejun@gmail.com> wrote:

> -	fd		= open(filename, O_RDONLY);
> +	/*
> +	 * Be careful! We are opening host block device!
> +	 * Open it readonly since we do not want to break user's data on disk.
> +	 */
> +	fd			= open(filename, O_RDONLY);
>  	if (fd < 0)
>  		return NULL;

btw., this is a repeating pattern i noticed: you align assignment vertically 
even if it's a stand-alone assignment.

We want to apply vertical alignment only when it helps readability - and 
repeated assingments such as:

        *job = (struct thread_pool__job) {
                .kvm            = kvm,
                .data           = data,
                .callback       = callback,
                .mutex          = PTHREAD_MUTEX_INITIALIZER
        };

indeed look *much* better when aligned vertically. Same goes for structure 
definitions. Thanks for applying those concepts uniformly around tools/kvm/,
it makes the code visibly more pleasant to read.

But the above standalone assignment of 'fd' does not seem to be such a case: 
the right side of the assignment just 'floats' freely in space with no other 
similar assingment next to it giving it structure.

Thus the old-fashioned:

	fd = open(filename, O_RDONLY);
 	if (fd < 0)
 		return NULL;

is a lot more readable form IMO.

There's many similar examples of isolated assignments looking weird, all around 
the code.

Thanks,

	Ingo