From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH v3] kvm tools: Add QCOW level2 caching support
Date: Fri, 3 Jun 2011 09:15:02 +0200
Message-ID: <20110603071502.GD15375@elte.hu>
References: <1307044916-18109-1-git-send-email-prasadjoshi124@gmail.com>
 <1307049561.13088.2.camel@lappy>
 <BANLkTi=KWU=Fis-KmmObCVQu2tRwmfDmUw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Sasha Levin <levinsasha928@gmail.com>,
	Prasad Joshi <prasadjoshi124@gmail.com>, kvm@vger.kernel.org,
	asias.hejun@gmail.com, gorcunov@gmail.com,
	chaitanyakulkarni15@gmail.com, ashwini.kulkarni@gmail.com,
	anupshendkar@gmail.com
To: Pekka Enberg <penberg@kernel.org>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx3.mail.elte.hu ([157.181.1.138]:42635 "EHLO mx3.mail.elte.hu"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750912Ab1FCHPG (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 3 Jun 2011 03:15:06 -0400
Content-Disposition: inline
In-Reply-To: <BANLkTi=KWU=Fis-KmmObCVQu2tRwmfDmUw@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>


* Pekka Enberg <penberg@kernel.org> wrote:

> On Fri, Jun 3, 2011 at 12:19 AM, Sasha Levin <levinsasha928@gmail.com=
> wrote:
> >> +static int cache_table(struct qcow *q, u64 *table, u64 offset)
> >> +{
> >> + =A0 =A0 struct qcow_l2_cache *n;
> >> + =A0 =A0 struct rb_root *r =3D &q->root;
> >> + =A0 =A0 struct qcow_l2_cache *lru;
> >> +
> >> + =A0 =A0 n =3D calloc(1, sizeof(struct qcow_l2_cache));
> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sizeof(*n)
> > sizeof() should use the variable name itself, not the data type. Ch=
eck
> > out chapter 14 in 'Documentation/CodingStyle'.
>=20
> Well, it doesn't matter that much, to be honest. 'n' could use a=20
> better name, though - 'cache' or 'c'.

I personally prefer the sizeof(*cache) variant for a subtle reason:=20
because during review it's easier to match up local variable names=20
than to match up types.

=46or example when i review code i only look at the types once: i just=20
establish their main nature and attach any semantic meaning to the=20
local variable name.

So if 'later in the code' i see "sizeof(struct qcow_l2_cache)" i wont=20
know it intuitively whether it matches up with 'cache' or not. So for=20
example i might not notice such a bug:

	cache =3D calloc(1, sizeof(struct qcow_l1_cache));

(trivia: how many seconds does it take for you to notice the bug in=20
the above line? Note, you already have the advantage that you *know*=20
that there's a bug in that line :-)

Even if i noticed the bug, i'd notice it by looking back at the local=20
variables defininition block - which is a distraction from reading=20
the code flow itself.

But if it's written differently i will notice this bug immediately:

	cache =3D calloc(1, sizeof(*r));

i will even notice this pattern:

	cache =3D calloc(1, sizeof(cache));

I guess we could introduce a type-safe zalloc variant, something=20
like:

	cache =3D zalloc_t(*cache);

Which is a clever macro that allocates sizeof(struct qcow_l2_cache)=20
bytes and gives back a 'struct qcow_l2_cache *' typed result. That=20
way this:

	cache =3D zalloc_t(r);

or this:

	cache =3D zalloc_t(cache);

Will result in a compiler error.

Hm?

Thanks,

	Ingo