On Tue, Jul 01, 2014 at 05:45:19PM +0800, Ding xiao wrote: Sorry for the delay, I forgot about this patch.
> +typedef struct vhd_tdbatmap_header {
> + char magic[8]; /* "tdbatmap"*/
> +
> + /* byte offset to batmap*/
> + uint64_t batmap_offset;
> +
> + /* Offset of the Block Allocation Table (BAT)*/
This comment describes the batmap_offset field? Maybe this should be
dropped since that field already has a comment.
> + /* read backend file*/
> + if (dyndisk_header->parent_name[0] ||
> dyndisk_header->parent_name[1]) {
> + for (i = 0; i < PARENT_LOCATOR_NUM; i++) {
> + data_offset = be64_to_cpu(
> +
> dyndisk_header->parent_locator[i].data_offset);
> + data_length = be32_to_cpu(
> +
> dyndisk_header->parent_locator[i].data_length);
> + if (dyndisk_header->parent_locator[i].platform == MACX) {
Missing be32_to_cpu()?
> + ret = bdrv_pread(bs->file, data_offset + 7,
> + bs->backing_file, data_length - 7);
Buffer overflow: char bs->backing_file[1024].
All input must be validated!
> + if (ret < 0) {
> + goto fail;
> + }
> + bs->backing_file[data_length - 7] = '\0';
Memory corruption if data_length < 7. Missing input validation.
> + }
> + if (data_offset > parent_locator_offset) {
> + parent_locator_offset = data_offset;
> + }
> + }
> + }
> +
> + if (parent_locator_offset + 512 > s->free_data_block_offset) {
> + s->free_data_block_offset = parent_locator_offset + 512;
> + }
> +
> for (i = 0; i < s->max_table_entries; i++) {
> be32_to_cpus(&s->pagetable[i]);
> if (s->pagetable[i] != 0xFFFFFFFF) {
> @@ -364,6 +425,9 @@ static inline int64_t get_sector_offset(BlockDriverState
> *bs,
> // bitmap each time we write to a new block. This might cause Virtual PC
> to
> // miss sparse read optimization, but it's not a problem in terms of
> // correctness.
> +
> + /*this will not use*/
> +#if 0
Delete the code if it is no longer used.
> @@ -433,7 +498,7 @@ static int rewrite_footer(BlockDriverState* bs)
> *
> * Returns the sectors' offset in the image file on success and < 0 on error
> */
> -static int64_t alloc_block(BlockDriverState* bs, int64_t sector_num)
> +static int64_t alloc_block(BlockDriverState *bs, int64_t sector_num, int
> diff)
Please use the C99 bool type since it communicates more clearly that
diff is either true or false (not a counter or bitmap).
It would be clearest to check footer->type for VHD_DIFFERENCING in this
function instead of adding a new argument to the function.
> @@ -501,33 +611,64 @@ static int vpc_read(BlockDriverState *bs, int64_t
> sector_num,
> int64_t offset;
> int64_t sectors, sectors_per_block;
> VHDFooter *footer = (VHDFooter *) s->footer_buf;
> + QEMUIOVector hd_qiov;
> + struct iovec qiov;
>
> if (cpu_to_be32(footer->type) == VHD_FIXED) {
> return bdrv_read(bs->file, sector_num, buf, nb_sectors);
> - }
> - while (nb_sectors > 0) {
> - offset = get_sector_offset(bs, sector_num, 0);
> + } else if (cpu_to_be32(footer->type) == VHD_DYNAMIC) {
cpu_to_be32() is wrong since VHD_DYNAMIC is an enum constant (just a
regular CPU-endian integer).
Please add a separate patch before this one that cleans up incorrect
cpu_to_be*() usage. For example, cpu_to_be32(footer->type) == VHD_FIXED
a few lines above is wrong too.
> + while (nb_sectors > 0) {
> + offset = get_sector_offset(bs, sector_num, 0);
> +
> + sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
> + sectors = sectors_per_block - (sector_num % sectors_per_block);
> + if (sectors > nb_sectors) {
> + sectors = nb_sectors;
> + }
>
> - sectors_per_block = s->block_size >> BDRV_SECTOR_BITS;
> - sectors = sectors_per_block - (sector_num % sectors_per_block);
> - if (sectors > nb_sectors) {
> - sectors = nb_sectors;
> + if (offset == -1) {
> + memset(buf, 0, sectors * BDRV_SECTOR_SIZE);
> + } else {
> + ret = bdrv_pread(bs->file, offset, buf,
> + sectors * BDRV_SECTOR_SIZE);
> + if (ret != sectors * BDRV_SECTOR_SIZE) {
Indentation is off
> + return -1;
> + }
> + }
> +
> + nb_sectors -= sectors;
> + sector_num += sectors;
> + buf += sectors * BDRV_SECTOR_SIZE;
> }
> + } else {
> + while (nb_sectors > 0) {
> + offset = get_sector_offset_diff(bs, sector_num);
> + if (offset == -1) {
> + memset(buf, 0, BDRV_SECTOR_SIZE);
> + } else if (offset == -2) {
> + qiov.iov_base = (void *)buf;
This cast is unnecessary. The compiler does not warn about pointer
casts to or from void*.
> + qiov.iov_len = 512;
> + hd_qiov.iov = &qiov;
> + hd_qiov.niov = 1;
> + hd_qiov.nalloc = -1;
> + hd_qiov.size = 512;
This is not idiomatic. Normally 'qiov' is a QEMUIOVector, not a struct
iovec. The qemu_iovec_*() functions should be used instead of manually
setting QEMUIOVector fields:
iov.iov_base = buf;
iov.iov_len = 512;
qemu_iovec_init_external(&hd_qiov, &iov, 1);
> + ret = bdrv_co_readv(bs->backing_hd, sector_num, 1, &hd_qiov);
> + if (ret < 0) {
> + return -1;
> + }
Why are you using bdrv_co_readv() instead of bdrv_pread() like the rest
of this file?
It would be simpler to use bdrv_pread(). Then you don't need the struct
iovec and QEMUIOVector. This function also hasn't been marked
coroutine_fn yet, so it is cleaner to stick with bdrv_pread() until the
file is properly converted to coroutines.
pgpTmDV4GVtP8.pgp
Description: PGP signature
