From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QR1XM-00020H-5g for qemu-devel@nongnu.org; Mon, 30 May 2011 08:25:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QR1XL-00076w-6V for qemu-devel@nongnu.org; Mon, 30 May 2011 08:25:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7452) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QR1XK-00076j-Sk for qemu-devel@nongnu.org; Mon, 30 May 2011 08:25:35 -0400 Message-ID: <4DE38D66.40903@redhat.com> Date: Mon, 30 May 2011 14:28:22 +0200 From: Kevin Wolf MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] VMDK: add monolithic flat image support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, Christoph Hellwig Am 30.05.2011 09:49, schrieb Fam Zheng: > VMDK multiple file images can not be recognized for now. This patch is > adding monolithic flat support to it, that is the image type with two > files, one text descriptor file and a plain data file. This type of > image can be created in VMWare, with the options "allocate all disk > space now" and "store virtual disk as a single file" checked. > > A VmdkExtent structure is introduced to hold the image "extent" > information, which makes further adding multi extents support of VMDK > easy. An image creating option "flat" is added for creating flat > (preallocated) image. > > Signed-off-by: Feiran (Fam) Zheng Ok, seems I commented on so many details that in the end I forgot to add the general comment. :-) I think this patch is too big to be well reviewable. You should always try to make only one logical change in one patch. I think you can split this at least in two parts: First adding the VmdkExtent data structure without adding any new functionality, and second adding the monolithic flat support. Depending on how big the second patch is, you can split it further into image creation and the rest (or any other split that you feel is natural). On two or three hunks I commented that they probably aren't required for monolithic flat support. They should be separate bugfix or cleanup patches. Kevin