From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36316) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRHHh-0007hE-1Z for qemu-devel@nongnu.org; Tue, 09 Sep 2014 05:00:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XRHHa-0003OT-TG for qemu-devel@nongnu.org; Tue, 09 Sep 2014 05:00:20 -0400 Received: from mail.univention.de ([82.198.197.8]:1074) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XRHHa-0003OK-Nc for qemu-devel@nongnu.org; Tue, 09 Sep 2014 05:00:14 -0400 Message-ID: <540EC19C.6070809@univention.de> Date: Tue, 09 Sep 2014 11:00:12 +0200 From: Philipp Hahn MIME-Version: 1.0 References: <1410187299-8463-1-git-send-email-gordon@localhost.localdomain> In-Reply-To: <1410187299-8463-1-git-send-email-gordon@localhost.localdomain> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] Support vhd type VHD_DIFFERENCING List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiaodong Gong , kwolf@redhat.com Cc: qemu-devel@nongnu.org, stefanha@redhat.com Hello, I'm no qemu-devel expert, but as I tried myself at adding VHD_DIFF support some time ago, here are some comments: On 08.09.2014 16:41, Xiaodong Gong wrote: > diff --git a/block/vpc.c b/block/vpc.c ... > + /* Read backing file location from dyn header table */ > + if (dyndisk_header->parent_name[0] || dyndisk_header->parent_name[1]) { > + for (i = 0; i < PARENT_LOCATOR_NUM; i++) { ... > + if (MACX == platform) { Most images I have luckily have the MACX entry, but from reading the spec I think this is not guaranteed. What about the other more Windows-agnostic types? There's also dyndisk_header->parent_name (which uses UTF-16-BE). > + ret = bdrv_pread(bs->file, data_offset + PARENT_PREFIX_LEN, > + bs->backing_file, data_length - PARENT_PREFIX_LEN); You assume that the system locale is UTF-8 (which MACX uses as the encoding). I don't know how QEMU handles that internally, but AFAIK for correctness you would need to convert that from UTF-8 to $LC_CTYPE. Using iconv() is currently is not possible, since it requires calling setlocale() to be called from all main programs using that code. > static int vpc_write(BlockDriverState *bs, int64_t sector_num, ... > + bool isdiff = true; ... > + if (true == isdiff) { if (isdiff) { is enough - no need to add any confusing ==true IMHO. Other notes: - Using backing files requires CONFIG_UUID. I once created a VHD file using qemu-img, which set UUID:=0. This lead to Xen handling the image as a raw-file instead of a vhd file instead. Otherwise thank you for working on VHD support. Sincerely Philipp