From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51048) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPpaX-00035e-U9 for qemu-devel@nongnu.org; Fri, 05 Sep 2014 05:13:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XPpaR-0000Kw-EA for qemu-devel@nongnu.org; Fri, 05 Sep 2014 05:13:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:13405) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XPpaR-0000Kl-6K for qemu-devel@nongnu.org; Fri, 05 Sep 2014 05:13:43 -0400 Date: Fri, 5 Sep 2014 11:13:35 +0200 From: Kevin Wolf Message-ID: <20140905091335.GC4656@noname.str.redhat.com> References: <1409821121-20645-1-git-send-email-stefanha@redhat.com> <54085604.2050005@redhat.com> <20140904135132.GC27130@stefanha-thinkpad.redhat.com> <20140904141014.GL3897@noname.str.redhat.com> <20140904154305.GA26494@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="h31gzZEtNLTqOjlF" Content-Disposition: inline In-Reply-To: <20140904154305.GA26494@stefanha-thinkpad.redhat.com> Subject: Re: [Qemu-devel] [PATCH] cow: make padding in the header explicit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: shhuiw@163.com, qemu-devel@nongnu.org, Stefan Hajnoczi --h31gzZEtNLTqOjlF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 04.09.2014 um 17:43 hat Stefan Hajnoczi geschrieben: > On Thu, Sep 04, 2014 at 04:10:14PM +0200, Kevin Wolf wrote: > > Am 04.09.2014 um 15:51 hat Stefan Hajnoczi geschrieben: > > > On Thu, Sep 04, 2014 at 06:07:32AM -0600, Eric Blake wrote: > > > > On 09/04/2014 02:58 AM, Stefan Hajnoczi wrote: > > > > > On-disk structures should be marked packed so the compiler does n= ot > > > > > insert padding for field alignment. Padding should be explicit so > > > > > on-disk layout is obvious and we don't rely on the architecture-s= pecific > > > > > ABI for alignment rules. > > > > >=20 > > > > > The pahole(1) diff shows that the padding is now explicit and off= sets > > > > > are unchanged: > > > > >=20 > > > > > char backing_file[1024]; /* 8 1024= */ > > > > > /* --- cacheline 16 boundary (1024 bytes) was 8 bytes ago --- */ > > > > > int32_t mtime; /* 1032 4= */ > > > > > - > > > > > - /* XXX 4 bytes hole, try to pack */ > > > > > - > > > > > + uint32_t padding; /* 1036 4= */ > > > > > uint64_t size; /* 1040 8= */ > > > >=20 > > > > Was a 32-bit build also inserting this padding, or do we have histo= rical > > > > differences where 32-bit and 64-bit cow files are actually differen= t, > > > > and we may need to be prepared to parse files from both sources? > > >=20 > > > Good point. Let's not merge this patch since it breaks 32-bit hosts. > > >=20 > > > The fact that no one hit problems when exchanging files between 32-bit > > > and 64-bit machines shows that the cow format is rarely used. > > >=20 > > > At this point we have 2 different formats: one without padding > > > (i386-style) and one with padding (x86_64-style). The chance of more > > > variants is small but who knows, maybe some other host architecture A= BI > > > has yet another alignment rule for uint64_t. > > >=20 > > > I'd like to git rm block/cow.c but I suppose the backwards-compatible > > > thing to do is to introduce subformats to support both variants. > > > Opinions? > >=20 > > Can we safely detect which of the subformats we have? But I'm not sure > > if it's even worth fixing. >=20 > I think it would default to the subformat depending on the host > architecture but allow overriding with -o subformat=3Di386|x86_64. Hm, okay. If we can't do it automatically, that's an option, too. > I'm also not sure if it's worth fixing. The cow file format is so > rarely used I wonder if we'd be better off without it. It has never been used as a native qemu image format. As I understand it, its use case is compatibility with existing images, and one of the great things about qemu-img is that it can open more or less any random image that you get from somewhere. The exotic formats are part of this. If we wanted to simplify our code, we could probably make it read-only (at the cost of losing qemu-iotests support), but then it's so simple that leaving r/w support around won't hurt us. Hm, actually, looking at the kernel (arch/um/drivers/cow_user.c), it doesn't look as if we were compatible at all for v2, at least in the current kernel version, we use a different backing file name length... And they have the struct packed today, so we should probably follow their example. On the other hand, out of three versions, we only support v2, and we don't use the same format as the kernel. Yeah, I guess we might just drop the support then, it's not helpful for compatibility. The only other way would be to update it to handle all three versions the same as the kernel code. Kevin --h31gzZEtNLTqOjlF Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJUCX6/AAoJEH8JsnLIjy/WBLYP/3/uX44nhQgPl0Y5kygytzZY GlBudqOdwPGlpl0mkT9AnGWHJeB+6ChkUpdAHcgP0Os6PZDtiN1UTL6zL6tEBmlg /WnTFQXph3QF3V97AY7iZ2FfIq/nEUW57B2npFSYemIEz2KOGvlGQ1ZJvHY5xgYM NhJDZj41jT+kjqYS7+DG9s8YUDOnn66slEppRt/5Vme4VLONS7EAbtnQzoF1c9IS Ozn7avOxy2m7cXfdxQgXO9nVGr70bb09hORfl9bScWvGmQgktwFU6xmqRwTDwtzP te6unBBki51OERp5a0xPQedLC3BjNC+yVaDKjiufCOWAtAGh+vKCk1cq4nS6067d PR4U73vVFkQAGt5pCfgTLzz+I9EPaY7GbyDs4KtcqpxPFa5oGr1jAsSBoJakrXA+ EVUV7X2+qws82gdht68qRurYJzQWgu71uj1nhjflez5Cyb64a/pWPwOKRCnebwPC SGU3II8d8nzAF+HtM+WpLWVb7IEbE3Z2fVQQ1DXHaZyLte59KHa6hVTbOoEc3R1k pnkL1gik8xzzR3+1yC3YkhIAJ0M9kKVo/qj++vpuC+GjJczUya1RKS+fvxam2wTl Oa1P38+qbmkSqwbzYDfBS1R8lwudoARtf7WLTSII0egvkTSwi1+BqrV0CrLXcd0F lSupRxxRz+7y4XR+QT+Q =bL4K -----END PGP SIGNATURE----- --h31gzZEtNLTqOjlF--