| Message ID | 1460553847-29886-2-git-send-email-rkx1209dev@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 13.04.2016 15:24, Ren Kimura wrote: > Signed-off-by: Ren Kimura <rkx1209dev@gmail.com> How about you put what you wrote in the 0/1 email here? (But with s/it's/its/ :-)) > --- > qemu-img.c | 27 ++++++++++++++++++++++++--- > 1 file changed, 24 insertions(+), 3 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 06264d9..53471a1 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1451,6 +1451,21 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num) > } > } > > +static int64_t get_backing_status(BlockDriverState *bs, > + int64_t sector_num, > + int nb_sectors, int *pnum, > + BlockDriverState **file) > +{ > + int64_t ret; > + if (bs->backing) { > + ret = get_backing_status(bs->backing->bs, sector_num, > + nb_sectors, pnum, file); This won't really work. You may not fall through to the backing file if the overlay file has an allocated range here. So you'll have to call bdrv_get_block_status() unconditionally first, and only if its return value has neither the BDRV_BLOCK_DATA nor the BDRV_BLOCK_ZERO flag set you can (and then have to) fall through to its backing file. > + } else { > + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, file); > + } > + return ret; > +} > + Apart from that, I'd suggest to drop the "file" parameter. We don't care about whatever is returned in it anyway. Just declare a local BlockDriverState pointer in this function which will receive this value. The reason for why I'm suggesting this is because this would make it clearer to the reader that this value is discarded. Feel free to follow or ignore this suggestion, based on whether you consider it useful or not. Max > static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > { > int64_t ret; > @@ -1469,7 +1484,6 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > if (ret < 0) { > return ret; > } > - > if (ret & BDRV_BLOCK_ZERO) { > s->status = BLK_ZERO; > } else if (ret & BDRV_BLOCK_DATA) { > @@ -1477,10 +1491,17 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > } else if (!s->target_has_backing) { > /* Without a target backing file we must copy over the contents of > * the backing file as well. */ > - /* TODO Check block status of the backing file chain to avoid > + /* Check block status of the backing file chain to avoid > * needlessly reading zeroes and limiting the iteration to the > * buffer size */ > - s->status = BLK_DATA; > + ret = get_backing_status(blk_bs(s->src[s->src_cur]), > + sector_num - s->src_cur_offset, > + n, &n, &file); > + if (ret & BDRV_BLOCK_ZERO) { > + s->status = BLK_ZERO; > + } else { > + s->status = BLK_DATA; > + } > } else { > s->status = BLK_BACKING_FILE; > } >
Max Thank you for reviewing. OK. I'll fix these and send version 2 later. Thanks
diff --git a/qemu-img.c b/qemu-img.c index 06264d9..53471a1 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1451,6 +1451,21 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num) } } +static int64_t get_backing_status(BlockDriverState *bs, + int64_t sector_num, + int nb_sectors, int *pnum, + BlockDriverState **file) +{ + int64_t ret; + if (bs->backing) { + ret = get_backing_status(bs->backing->bs, sector_num, + nb_sectors, pnum, file); + } else { + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, file); + } + return ret; +} + static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) { int64_t ret; @@ -1469,7 +1484,6 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) if (ret < 0) { return ret; } - if (ret & BDRV_BLOCK_ZERO) { s->status = BLK_ZERO; } else if (ret & BDRV_BLOCK_DATA) { @@ -1477,10 +1491,17 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) } else if (!s->target_has_backing) { /* Without a target backing file we must copy over the contents of * the backing file as well. */ - /* TODO Check block status of the backing file chain to avoid + /* Check block status of the backing file chain to avoid * needlessly reading zeroes and limiting the iteration to the * buffer size */ - s->status = BLK_DATA; + ret = get_backing_status(blk_bs(s->src[s->src_cur]), + sector_num - s->src_cur_offset, + n, &n, &file); + if (ret & BDRV_BLOCK_ZERO) { + s->status = BLK_ZERO; + } else { + s->status = BLK_DATA; + } } else { s->status = BLK_BACKING_FILE; }
Signed-off-by: Ren Kimura <rkx1209dev@gmail.com> --- qemu-img.c | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)