Re: [Qemu-devel] [PATCH RESEND] Support vhd type VHD_DIFFERENCING

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3471 bytes --]

On Thu, Nov 06, 2014 at 10:43:50PM +0800, Xiaodong Gong wrote:
> +        } else if (platform == PLATFORM_W2RU) {
> +            /* Must be UTF16-LE to ASCII */
> +            char *out, *optr;
> +            int j;
> +
> +            optr = out = (char *) g_malloc(data_length + 1);
> +            if (out == NULL) {
> +                ret = -1;
> +                return ret;
> +            }
> +
> +            for (j = 0; j < data_length + 1; j++) {
> +                out[j] = bs->backing_file[2 * j];
> +            }
> +            out[data_length + 1] = '\0';
> +
> +            while (*optr != '\0') {
> +                if (*optr == '\\') {
> +                    *optr = '/';
> +                }
> +                optr++;
> +            }
> +
> +            strncpy(bs->backing_file, out, data_length + 1);
> +
> +            g_free(out);
> +            out = NULL;
> +
> +            done = true;
> +        }

Please convert from UTF-16 LE to the local file system character set:
https://developer.gnome.org/glib/stable/glib-Character-Set-Conversion.html

Also, using ->backing_file[] when the data is UTF-16 LE encoded is not
ideal since it halves the maximum size of the string!  It would be
better to read into a temporary buffer that is 2 *
sizeof(backing_file[]) big before writing into ->backing_file[].

> @@ -286,6 +411,37 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>          s->free_data_block_offset =
>              (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511;
>  
> +        /* Read tdbatmap header by offset */
> +        if (footer->version >= VHD_VERSION(1, 2)) {

Missing be32_to_cpu(footer->version)

> +static int vpc_write(BlockDriverState *bs, int64_t sector_num,
> +    const uint8_t *buf, int nb_sectors)
> +{
> +    BDRVVPCState *s = bs->opaque;
> +    VHDFooter *footer = (VHDFooter *) s->footer_buf;
> +    int64_t sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
> +    int64_t offset, sectors;
> +    bool diff = true;
> +    int ret = 0;
> +
> +    switch (be32_to_cpu(footer->type)) {
> +    case VHD_FIXED:
> +        return bdrv_write(bs->file, sector_num, buf, nb_sectors);
> +    case VHD_DYNAMIC:
> +    case VHD_DIFF:
> +        if (be32_to_cpu(footer->type) == VHD_DYNAMIC) {
> +            diff = false;
>          }

This can be done with a fall-through case instead of checking
footer->type again:

case VHD_DYNAMIC:
    diff = false;
    /* fall-through */
case VHD_DIFF:

> +            ret = bdrv_pwrite(bs->file, offset, buf,
> +                sectors * BDRV_SECTOR_SIZE);
> +            if (ret != sectors * BDRV_SECTOR_SIZE) {
> +                return -1;
> +            }
>  
> -        nb_sectors -= sectors;
> -        sector_num += sectors;
> -        buf += sectors * BDRV_SECTOR_SIZE;
> -    }
> +            if (diff) {
> +                ret = write_bitmap(bs, sector_num, sectors);
> +                if (ret < 0) {
> +                    return -1;
> +                }
> +            }
>  
> -    return 0;
> +            nb_sectors -= sectors;
> +            sector_num += sectors;
> +            buf += sectors * BDRV_SECTOR_SIZE;
> +        }
> +        break;
> +    default:
> +        return -1;
> +    }
> +    return ret;

In the VHD_DYNAMIC case we must *not* return the number of bytes from
bdrv_pwrite().  Should this be return 0?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]