From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39545) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0TTT-0005xN-As for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:06:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0TTM-0005NK-TL for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:05:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38138) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0TTM-0005NA-LT for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:05:52 -0500 Date: Mon, 15 Dec 2014 12:05:48 +0100 From: Kevin Wolf Message-ID: <20141215110548.GD4411@noname.str.redhat.com> References: <1418632081-20667-1-git-send-email-den@openvz.org> <1418632081-20667-6-git-send-email-den@openvz.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1418632081-20667-6-git-send-email-den@openvz.org> Subject: Re: [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 15.12.2014 um 09:27 hat Denis V. Lunev geschrieben: > Unfortunately, old guest OSes do not align partitions to page size by > default. This is true for Windows 2003 and Windows XP. > > For the time being Parallels was created an optimization for such OSes > in its desktop product. Desktop users are not qualified enough to create > properly aligned installations. Thus Parallels makes a blind guess > on a customer behalf and creates so-called "padded" images if guest > OS type is specified as WinXP, Win2k and Win2k3. > > "Padding" is a value which should be added to guest LBA to obtain > sector number inside the image. This results in a shifted images. > 0123 offset inside image (in 512 byte sectors) > +------- > +.012 guest data (512 byte sectors) > +------- > The information about this is available in DiskDescriptor.xml ONLY. There > is no such data in the image header. > > There share of such images could be evaluated as 6-8% according to the > statistics in my hands. > > This patch obtains proper value from XML and applies it on reading. > > Signed-off-by: Denis V. Lunev > Acked-by: Roman Kagan > Reviewed-by: Jeff Cody > CC: Kevin Wolf > CC: Stefan Hajnoczi > --- > block/parallels.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/block/parallels.c b/block/parallels.c > index c22b91b..fedb009 100644 > --- a/block/parallels.c > +++ b/block/parallels.c > @@ -63,6 +63,7 @@ typedef struct BDRVParallelsState { > unsigned int tracks; > > unsigned int off_multiplier; > + unsigned int padding; > } BDRVParallelsState; > > > @@ -235,6 +236,7 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp) > const char *data; > char image_path[PATH_MAX]; > Error *local_err = NULL; > + BDRVParallelsState *s = bs->opaque; > > ret = size = bdrv_getlength(bs->file); > if (ret < 0) { > @@ -264,6 +266,19 @@ static int parallels_open_xml(BlockDriverState *bs, int flags, Error **errp) > if (root == NULL) { > goto fail; > } > + > + data = xml_get_text(root, "Disk_Parameters", "Padding", NULL); > + if (data != NULL) { > + char *endptr; > + unsigned long pad; > + > + pad = strtoul(data, &endptr, 0); > + if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) { Can endptr even be NULL? Also, shouldn't you set errno = 0 before and check it here? > + goto fail; > + } > + s->padding = (uint32_t)pad; s->padding is unsigned int, pad is unsigned long. Why the cast to uint32_t here, which is different from both? > + } > + > image = xml_seek(root, "StorageData", "Storage", "Image", NULL); > data = ""; /* make gcc happy */ > for (size = 0; image != NULL; image = image->next) { > @@ -365,6 +380,10 @@ static int64_t seek_to_sector(BlockDriverState *bs, int64_t sector_num) > static int parallels_read(BlockDriverState *bs, int64_t sector_num, > uint8_t *buf, int nb_sectors) > { > + BDRVParallelsState *s = bs->opaque; > + > + sector_num += s->padding; No check needed here? bdrv_check_request() has checked sector_num and nb_sectors against the image size, but now you can't rely on the result any more. > while (nb_sectors > 0) { > int64_t position = seek_to_sector(bs, sector_num); > if (position >= 0) { Kevin