Re: [Qemu-devel] [PATCH 05/16] block/parallels: support padded Parallels images

[Denis V. Lunev]

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 <address@hidden>
> > Acked-by: Roman Kagan <address@hidden>
> > Reviewed-by: Jeff Cody <address@hidden>
> > CC: Kevin Wolf <address@hidden>
> > CC: Stefan Hajnoczi <address@hidden>
> > ---
> >   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