2015年2月25日 22:58于 "Stefan Hajnoczi" <stefa...@gmail.com>写道: > > 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 <gongxiaodo...@huawei.com> > > Reviewed-by: Ding xiao <ssdx...@163.com> > > --- > > 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 >
iconv is a buildin fuction in libc.And I greped,but no use of g_conv*. I will exchange icovn with g_conv > > #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. > It is the source of mistake,I will change it to value to graft > > +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. > yes > 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. > the same as the upper commant of “big ending” > > + 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. > This var is a water line of offset,it is directly a uint64. Is this the worrest code I ever did? > > @@ -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? > yes,to pass checkpatch > > @@ -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. > yes > > @@ -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? recover it