|
| From: | Xiaodong Gong |
| Subject: | Re: [Qemu-devel] [PATCH v9] Support vhd type VHD_DIFFERENCING |
| Date: | Fri, 27 Feb 2015 13:49:09 +0800 |
2015年2月25日 22:58于 "Stefan Hajnoczi" <address@hidden>写道：
>
> 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
>
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
| [Prev in Thread] | Current Thread | [Next in Thread] |