From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=39768 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1P6YFv-00070E-4F
	for qemu-devel@nongnu.org; Thu, 14 Oct 2010 20:34:44 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <yehudasa@gmail.com>) id 1P6YFu-0007Xj-82
	for qemu-devel@nongnu.org; Thu, 14 Oct 2010 20:34:43 -0400
Received: from mail-ww0-f53.google.com ([74.125.82.53]:59406)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <yehudasa@gmail.com>) id 1P6YFu-0007Xe-1t
	for qemu-devel@nongnu.org; Thu, 14 Oct 2010 20:34:42 -0400
Received: by wwc33 with SMTP id 33so287843wwc.10
	for <qemu-devel@nongnu.org>; Thu, 14 Oct 2010 17:34:41 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <AANLkTi=7oYpBRs6ZmpHm-KpsQFrH1ELNFgWRnPEYCQju@mail.gmail.com>
References: <20101012231854.GA4535@dell.home>
	<AANLkTi=7oYpBRs6ZmpHm-KpsQFrH1ELNFgWRnPEYCQju@mail.gmail.com>
Date: Thu, 14 Oct 2010 17:34:41 -0700
Message-ID: <AANLkTi=EhA=dT9ufuv10PgDLBX1hdDbD__tNGKnCtAW1@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH] ceph/rbd block driver for qemu-kvm (v6)
From: Yehuda Sadeh Weinraub <yehudasa@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Hajnoczi <stefanha@gmail.com>
Cc: kwolf@redhat.com, ceph-devel@vger.kernel.org, Christian Brunner <chb@muc.de>, kvm@vger.kernel.org, qemu-devel@nongnu.org

See my comments:

On Wed, Oct 13, 2010 at 1:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote=
:
>> +
>> + =A0 =A0cpu_to_le32s(&snap_count);
>> + =A0 =A0cpu_to_le64s(&snap_names_len);

Redone all endianity conversions, made it so that it keeps the header
as little endian, and whenever reading the header, do the endianity
conversion.

>
> It is clearer to do byteswapping immediately, rather than having the
> variable take on different endianness at different times:
> uint32_t snap_count =3D cpu_to_le32(header->snap_count);
> uint64_t snap_names_len =3D cpu_to_le64(header->snap_names_len);

Right.

>
>> + =A0 =A0if (snap_count) {
>> + =A0 =A0 =A0 =A0const char *header_snap =3D (const char *)&header->snap=
s[snap_count];
>> + =A0 =A0 =A0 =A0const char *end =3D header_snap + snap_names_len;
>
> snap_names_len is little-endian. =A0This won't work on big-endian hosts.
> =A0Did you mean le64_to_cpu() instead of cpu_to_le64()?
Yes, fixed that.

>
>> + =A0 =A0 =A0 =A0snaps =3D qemu_malloc(sizeof(rados_snap_t) * header->sn=
ap_count);
>
> snaps is allocated here...
>
>> +
>> + =A0 =A0 =A0 =A0for (i=3D0; i < snap_count; i++) {
>> + =A0 =A0 =A0 =A0 =A0 =A0snaps[i] =3D (uint64_t)header->snaps[i].id;
>> + =A0 =A0 =A0 =A0 =A0 =A0cpu_to_le64s(&snaps[i]);
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0if (snap && strcmp(snap, header_snap) =3D=3D 0)=
 {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0snapid =3D snaps[i];
>> + =A0 =A0 =A0 =A0 =A0 =A0}
>> +
>> + =A0 =A0 =A0 =A0 =A0 =A0header_snap +=3D strlen(header_snap) + 1;
>> + =A0 =A0 =A0 =A0 =A0 =A0if (header_snap > end) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0error_report("bad header, snapshot list=
 broken");
>> + =A0 =A0 =A0 =A0 =A0 =A0}
>> + =A0 =A0 =A0 =A0}
>> + =A0 =A0}
>> +
>> + =A0 =A0if (snap && !snapid) {
>> + =A0 =A0 =A0 =A0error_report("snapshot not found");
>> + =A0 =A0 =A0 =A0return -ENOENT;
>
> ...but never freed here.

Freed now.



Thanks,
Yehuda