Re: [Qemu-devel] [PATCH 4/6] block/parallels: support padded Parallels images

[Jeff Cody <jcody@redhat.com>]

On Wed, Oct 29, 2014 at 04:38:09PM +0300, Denis V. Lunev wrote:
> 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 <den@openvz.org>
> Acked-by: Roman Kagan <rkagan@parallels.com>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/parallels.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 201c0f1..d07e4f7 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -66,6 +66,7 @@ typedef struct BDRVParallelsState {
>      unsigned int tracks;
>  
>      unsigned int off_multiplier;
> +    unsigned int padding;
>  } BDRVParallelsState;
>  
>  static int parallels_open_image(BlockDriverState *bs, Error **errp);
> @@ -144,6 +145,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) {
> @@ -173,6 +175,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");
> +    if (data != NULL) {
> +        char *endptr;
> +        unsigned long pad;
> +
> +        pad = strtoul(data, &endptr, 0);
> +        if ((endptr != NULL && *endptr != '\0') || pad > UINT_MAX) {
> +            goto fail;
> +        }
> +        s->padding = (uint32_t)pad;
> +    }
> +
>      image = xml_seek(root, "/StorageData/Storage/Image");
>      data = ""; /* make gcc happy */
>      for (size = 0; image != NULL; image = image->next) {
> @@ -385,6 +400,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;
> +
>      while (nb_sectors > 0) {
>          int64_t position = seek_to_sector(bs, sector_num);
>          if (position >= 0) {
> -- 
> 1.9.1
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>