From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46169) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Tuv-0005WG-SO for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:34:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0Tuq-0004lB-Al for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:34:21 -0500 Received: from relay.parallels.com ([195.214.232.42]:53637) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Tuq-0004l0-0Y for qemu-devel@nongnu.org; Mon, 15 Dec 2014 06:34:16 -0500 Message-ID: <548EC727.3030908@openvz.org> Date: Mon, 15 Dec 2014 14:33:59 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1418632081-20667-1-git-send-email-den@openvz.org> <1418632081-20667-6-git-send-email-den@openvz.org> <20141215110548.GD4411@noname.str.redhat.com> In-Reply-To: <20141215110548.GD4411@noname.str.redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit 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: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi On 15/12/14 14:05, Kevin Wolf wrote: > 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? got it, I think you are right >> + 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? ok >> + } >> + >> 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. you are perfectly correct. I'll recheck the situation. I have seen some dances with image size. Allowed image size should be extended with 1 block, but I am not quite sure. Thank you for pointing this out. >> while (nb_sectors > 0) { >> int64_t position = seek_to_sector(bs, sector_num); >> if (position >= 0) { > Kevin