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

[Stefan Hajnoczi]

On Sun, Feb 15, 2015 at 09:35:49PM +0800, Xiaodong Gong wrote:
> Now qemu only supports vhd type VHD_FIXED and VHD_DYNAMIC, so qemu
> can't read snapshot volume of vhd, and can't support other storage
> features of vhd file.
> 
> This patch add read parent information in function "vpc_open", read
> bitmap in "vpc_read", and change bitmap in "vpc_write".
> 
> Signed-off-by: Xiaodong Gong <address@hidden>
> Reviewed-by: Ding xiao <address@hidden>
> ---
> Changes since v8
> - use backing_format to avoid being probed to format of raw
> 
> Changes since v7:
> - use iconv to decode UTF-16LE(w2u) and UTF-8(macx) to ASCII
>   (Stefan Hajnoczi)

I suggested glib's character set conversion functions, not iconv.

glib abstracts the dependency character set conversion so it will work
across platforms.  That way we don't have to add iconv library detection
to ./configure.  Please use glib since QEMU already depends on it.

https://developer.gnome.org/glib/stable/glib-Character-Set-Conversion.html

>  #define HEADER_SIZE 512
> +#define DYNAMIC_HEADER_SIZE 1024
> +#define PARENT_LOCATOR_NUM 8
> +#define TBBATMAP_HEAD_SIZE 28
> +
> +#define MACX_PREFIX_LEN 7 /* file:// */
> +
> +#define PLATFORM_MACX 0x5863614d /* big endian */

This comment doesn't make sense.  The constant is just a C integer
literal, it doesn't have endianness.

> +static int vpc_decode_parent_loc(uint32_t platform,
> +                                 BlockDriverState *bs,
> +                                 int data_length)
> +{
> +    int ret;
> +
> +    switch (platform) {
> +    case PLATFORM_MACX:
> +        ret = vpc_decode_maxc_loc(bs, data_length);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        break;
> +
> +    case PLATFORM_W2RU:
> +        /* fall through! */
> +    case PLATFORM_W2KU:
> +        ret = vpc_decode_w2u_loc(bs, data_length);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        break;
> +
> +    default:
> +        return 0;

This should fail.  There are unimplemented platform codes.  We should
not attempt to open the file any further.

In the PLATFORM_NONE (0x0) case it may be cleanest to
vpc_read_backing_loc()'s for loop to skip empty platform locators
instead of trying to read 0 bytes and then calling
vpc_decode_parent_loc() with no data.

> +    }
> +
> +    return 0;
> +}
> +
> +static int vpc_read_backing_loc(VHDDynDiskHeader *dyndisk_header,
> +                                BlockDriverState *bs,
> +                                Error **errp)
> +{
> +    BDRVVPCState *s = bs->opaque;
> +    int64_t data_offset = 0;
> +    int data_length = 0;
> +    uint32_t platform;
> +    bool done = false;
> +    int parent_locator_offset = 0;
> +    int i;
> +    int ret = 0;
> +
> +    for (i = 0; i < PARENT_LOCATOR_NUM; i++) {
> +        /* The PLATFORM_* is big ending, and the dyndisk_header
> +         * is always big ending. So whatever this platform in cpu
> +         * is, it works. */

This comment is incorrect.  dyndisk_header is big endian but PLATFORM_*
is not big endian.  Please drop the comment.

> +        platform =
> +            dyndisk_header->parent_locator[i].platform;

Missing be32_to_cpu().

> +        data_offset =
> +            be64_to_cpu(dyndisk_header->parent_locator[i].data_offset);
> +        data_length =
> +            be32_to_cpu(dyndisk_header->parent_locator[i].data_length);
> +
> +        /* Extend the location offset */
> +        if (parent_locator_offset < data_offset) {
> +            parent_locator_offset = data_offset;

Why is parent_locator_offset and this function's return value int?

data_offset is uint64_t and it seems like values could get truncated if
int is used.

> @@ -278,7 +523,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, 
> int flags,
>          s->bat_offset = be64_to_cpu(dyndisk_header->table_offset);
>  
>          ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable,
> -                         s->max_table_entries * 4);
> +            s->max_table_entries * 4);
>          if (ret < 0) {
>              goto fail;
>          }

Unnecessary whitespace change?

> @@ -286,6 +531,48 @@ 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 (be32_to_cpu(footer->version) >= VHD_VERSION(1, 2)) {
> +            ret = bdrv_pread(bs->file, s->free_data_block_offset,
> +                tdbatmap_header_buf, TBBATMAP_HEAD_SIZE);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +
> +            tdbatmap_header = (VHDTdBatmapHeader *) tdbatmap_header_buf;
> +            if (!strncmp(tdbatmap_header->magic, "tdbatmap", 8)) {
> +                s->free_data_block_offset =
> +                    be32_to_cpu(tdbatmap_header->batmap_size) * 512
> +                    + be64_to_cpu(tdbatmap_header->batmap_offset);
> +            }
> +        }
> +
> +        if (dyndisk_header->parent_name[0] || 
> dyndisk_header->parent_name[1]) {
> +            int len;
> +
> +            /* Read parent location from dyn header table */
> +            ret = parent_locator_offset = 
> vpc_read_backing_loc(dyndisk_header,
> +                bs, errp);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +
> +            /* Fix me : Set parent format to avoid probing to raw in
> +             * format probe framework */
> +            len = strlen("vpc");
> +            if (sizeof(bs->backing_format) - 1 < len) {
> +                goto fail;
> +            }
> +            pstrcpy(bs->backing_format, sizeof(bs->backing_format), "vpc");
> +            bs->backing_format[len] = '\0';

pstrcpy() always NUL-terminates so this is not necessary.

> @@ -376,7 +704,7 @@ static inline int64_t get_sector_offset(BlockDriverState 
> *bs,
>          bdrv_pwrite_sync(bs->file, bitmap_offset, bitmap, s->bitmap_size);
>      }
>  
> -//    printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 
> ", bloff: %" PRIx64 "\n",
> +//  printf("sector: %" PRIx64 ", index: %x, offset: %x, bioff: %" PRIx64 ", 
> bloff: %" PRIx64 "\n",
>  //   sector_num, pagetable_index, pageentry_index,
>  //   bitmap_offset, block_offset);
>  

Unnecessary whitespace change?

pgpCIW4tVJB7s.pgp
Description: PGP signature