| Message ID | 1399628898-3241-4-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
Markus Armbruster <armbru@redhat.com> writes: > Instead of bdrv_nb_sectors(). > > Aside: a few of these callers don't handle errors. I didn't > investigate whether they should. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- [...] > diff --git a/block.c b/block.c > index 44e1f57..1b99cb1 100644 > --- a/block.c > +++ b/block.c [...] > @@ -3848,21 +3845,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > int64_t sector_num, > int nb_sectors, int *pnum) > { > - int64_t length; > + int64_t total_sectors; > int64_t n; > int64_t ret, ret2; > > - length = bdrv_getlength(bs); > - if (length < 0) { > - return length; > + total_sectors = bdrv_getlength(bs); > + if (total_sectors < 0) { > + return total_sectors; > } > > - if (sector_num >= (length >> BDRV_SECTOR_BITS)) { > + if (sector_num >= total_sectors) { > *pnum = 0; > return 0; > } > > - n = bs->total_sectors - sector_num; > + n = total_sectors - sector_num; > if (n < nb_sectors) { > nb_sectors = n; > } > @@ -3893,8 +3890,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > ret |= BDRV_BLOCK_ZERO; > } else if (bs->backing_hd) { > BlockDriverState *bs2 = bs->backing_hd; > - int64_t length2 = bdrv_getlength(bs2); > - if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) { > + int64_t nb_sectors2 = bdrv_getlength(bs2); > + if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) { > ret |= BDRV_BLOCK_ZERO; > } > } [...] I neglected to actually replace bdrv_getlength() by bdrv_nb_sectors() here, breaking test 030 (I forgot that make check-block doesn't run all the tests). With that fixed, the tests pass. Full respin wanted?
Am 09.05.2014 um 18:27 hat Markus Armbruster geschrieben: > Markus Armbruster <armbru@redhat.com> writes: > > > Instead of bdrv_nb_sectors(). > > > > Aside: a few of these callers don't handle errors. I didn't > > investigate whether they should. > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > --- > [...] > > diff --git a/block.c b/block.c > > index 44e1f57..1b99cb1 100644 > > --- a/block.c > > +++ b/block.c > [...] > > @@ -3848,21 +3845,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > > int64_t sector_num, > > int nb_sectors, int *pnum) > > { > > - int64_t length; > > + int64_t total_sectors; > > int64_t n; > > int64_t ret, ret2; > > > > - length = bdrv_getlength(bs); > > - if (length < 0) { > > - return length; > > + total_sectors = bdrv_getlength(bs); > > + if (total_sectors < 0) { > > + return total_sectors; > > } > > > > - if (sector_num >= (length >> BDRV_SECTOR_BITS)) { > > + if (sector_num >= total_sectors) { > > *pnum = 0; > > return 0; > > } > > > > - n = bs->total_sectors - sector_num; > > + n = total_sectors - sector_num; > > if (n < nb_sectors) { > > nb_sectors = n; > > } > > @@ -3893,8 +3890,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > > ret |= BDRV_BLOCK_ZERO; > > } else if (bs->backing_hd) { > > BlockDriverState *bs2 = bs->backing_hd; > > - int64_t length2 = bdrv_getlength(bs2); > > - if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) { > > + int64_t nb_sectors2 = bdrv_getlength(bs2); > > + if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) { > > ret |= BDRV_BLOCK_ZERO; > > } > > } > [...] > > I neglected to actually replace bdrv_getlength() by bdrv_nb_sectors() > here, breaking test 030 (I forgot that make check-block doesn't run all > the tests). With that fixed, the tests pass. Full respin wanted? Yes, please. I think you're aware of it, but in order to avoid misunderstandings let me mention that there are two places that need a fix here, for both total_sectors and nb_sectors2. Also please consider splitting this patch so that the functions with major changes (bdrv_make_zero, bdrv_co_get_block_status) get separated at least. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 09.05.2014 um 18:27 hat Markus Armbruster geschrieben: >> Markus Armbruster <armbru@redhat.com> writes: >> >> > Instead of bdrv_nb_sectors(). >> > >> > Aside: a few of these callers don't handle errors. I didn't >> > investigate whether they should. >> > >> > Signed-off-by: Markus Armbruster <armbru@redhat.com> >> > --- >> [...] >> > diff --git a/block.c b/block.c >> > index 44e1f57..1b99cb1 100644 >> > --- a/block.c >> > +++ b/block.c >> [...] >> > @@ -3848,21 +3845,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, >> > int64_t sector_num, >> > int nb_sectors, int *pnum) >> > { >> > - int64_t length; >> > + int64_t total_sectors; >> > int64_t n; >> > int64_t ret, ret2; >> > >> > - length = bdrv_getlength(bs); >> > - if (length < 0) { >> > - return length; >> > + total_sectors = bdrv_getlength(bs); >> > + if (total_sectors < 0) { >> > + return total_sectors; >> > } >> > >> > - if (sector_num >= (length >> BDRV_SECTOR_BITS)) { >> > + if (sector_num >= total_sectors) { >> > *pnum = 0; >> > return 0; >> > } >> > >> > - n = bs->total_sectors - sector_num; >> > + n = total_sectors - sector_num; >> > if (n < nb_sectors) { >> > nb_sectors = n; >> > } >> > @@ -3893,8 +3890,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, >> > ret |= BDRV_BLOCK_ZERO; >> > } else if (bs->backing_hd) { >> > BlockDriverState *bs2 = bs->backing_hd; >> > - int64_t length2 = bdrv_getlength(bs2); >> > - if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) { >> > + int64_t nb_sectors2 = bdrv_getlength(bs2); >> > + if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) { >> > ret |= BDRV_BLOCK_ZERO; >> > } >> > } >> [...] >> >> I neglected to actually replace bdrv_getlength() by bdrv_nb_sectors() >> here, breaking test 030 (I forgot that make check-block doesn't run all >> the tests). With that fixed, the tests pass. Full respin wanted? > > Yes, please. I think you're aware of it, but in order to avoid > misunderstandings let me mention that there are two places that need a > fix here, for both total_sectors and nb_sectors2. Correct. > Also please consider splitting this patch so that the functions with > major changes (bdrv_make_zero, bdrv_co_get_block_status) get separated > at least. Okay.
diff --git a/block-migration.c b/block-migration.c index 56951e0..f5bda8f 100644 --- a/block-migration.c +++ b/block-migration.c @@ -185,7 +185,7 @@ static int bmds_aio_inflight(BlkMigDevState *bmds, int64_t sector) { int64_t chunk = sector / (int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK; - if ((sector << BDRV_SECTOR_BITS) < bdrv_getlength(bmds->bs)) { + if (sector < bdrv_nb_sectors(bmds->bs)) { return !!(bmds->aio_bitmap[chunk / (sizeof(unsigned long) * 8)] & (1UL << (chunk % (sizeof(unsigned long) * 8)))); } else { @@ -222,8 +222,7 @@ static void alloc_aio_bitmap(BlkMigDevState *bmds) BlockDriverState *bs = bmds->bs; int64_t bitmap_size; - bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS) + - BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1; + bitmap_size = bdrv_nb_sectors(bs) + BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1; bitmap_size /= BDRV_SECTORS_PER_DIRTY_CHUNK * 8; bmds->aio_bitmap = g_malloc0(bitmap_size); @@ -349,7 +348,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs) int64_t sectors; if (!bdrv_is_read_only(bs)) { - sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS; + sectors = bdrv_nb_sectors(bs); if (sectors <= 0) { return; } @@ -797,7 +796,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id) if (bs != bs_prev) { bs_prev = bs; - total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS; + total_sectors = bdrv_nb_sectors(bs); if (total_sectors <= 0) { error_report("Error getting length of block device %s", device_name); diff --git a/block.c b/block.c index 44e1f57..1b99cb1 100644 --- a/block.c +++ b/block.c @@ -2784,18 +2784,16 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num, */ int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags) { - int64_t target_size; - int64_t ret, nb_sectors, sector_num = 0; + int64_t target_sectors, ret, nb_sectors, sector_num = 0; int n; - target_size = bdrv_getlength(bs); - if (target_size < 0) { - return target_size; + target_sectors = bdrv_nb_sectors(bs); + if (target_sectors < 0) { + return target_sectors; } - target_size /= BDRV_SECTOR_SIZE; for (;;) { - nb_sectors = target_size - sector_num; + nb_sectors = target_sectors - sector_num; if (nb_sectors <= 0) { return 0; } @@ -3012,15 +3010,14 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs, ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov); } else { /* Read zeros after EOF of growable BDSes */ - int64_t len, total_sectors, max_nb_sectors; + int64_t total_sectors, max_nb_sectors; - len = bdrv_getlength(bs); - if (len < 0) { - ret = len; + total_sectors = bdrv_nb_sectors(bs); + if (total_sectors < 0) { + ret = total_sectors; goto out; } - total_sectors = DIV_ROUND_UP(len, BDRV_SECTOR_SIZE); max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num), align >> BDRV_SECTOR_BITS); if (max_nb_sectors > 0) { @@ -3848,21 +3845,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { - int64_t length; + int64_t total_sectors; int64_t n; int64_t ret, ret2; - length = bdrv_getlength(bs); - if (length < 0) { - return length; + total_sectors = bdrv_getlength(bs); + if (total_sectors < 0) { + return total_sectors; } - if (sector_num >= (length >> BDRV_SECTOR_BITS)) { + if (sector_num >= total_sectors) { *pnum = 0; return 0; } - n = bs->total_sectors - sector_num; + n = total_sectors - sector_num; if (n < nb_sectors) { nb_sectors = n; } @@ -3893,8 +3890,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, ret |= BDRV_BLOCK_ZERO; } else if (bs->backing_hd) { BlockDriverState *bs2 = bs->backing_hd; - int64_t length2 = bdrv_getlength(bs2); - if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) { + int64_t nb_sectors2 = bdrv_getlength(bs2); + if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) { ret |= BDRV_BLOCK_ZERO; } } @@ -5178,13 +5175,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity, granularity >>= BDRV_SECTOR_BITS; assert(granularity); - bitmap_size = bdrv_getlength(bs); + bitmap_size = bdrv_nb_sectors(bs); if (bitmap_size < 0) { error_setg_errno(errp, -bitmap_size, "could not get length of device"); errno = -bitmap_size; return NULL; } - bitmap_size >>= BDRV_SECTOR_BITS; bitmap = g_malloc0(sizeof(BdrvDirtyBitmap)); bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); diff --git a/block/qcow2.c b/block/qcow2.c index a4b97e8..98f624c 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1539,7 +1539,7 @@ static int preallocate(BlockDriverState *bs) int ret; QCowL2Meta *meta; - nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS; + nb_sectors = bdrv_nb_sectors(bs); offset = 0; while (nb_sectors) { diff --git a/block/vmdk.c b/block/vmdk.c index 06a1f9f..eb2b03e 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -669,8 +669,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) { l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9; } - if (bdrv_getlength(file) < - le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) { + if (bdrv_nb_sectors(file) < le64_to_cpu(header.grain_offset)) { error_setg(errp, "File truncated, expecting at least %" PRId64 " bytes", (int64_t)(le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE)); @@ -1991,7 +1990,7 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result, BDRVVmdkState *s = bs->opaque; VmdkExtent *extent = NULL; int64_t sector_num = 0; - int64_t total_sectors = bdrv_getlength(bs) / BDRV_SECTOR_SIZE; + int64_t total_sectors = bdrv_nb_sectors(bs); int ret; uint64_t cluster_offset; diff --git a/qemu-img.c b/qemu-img.c index 96f4463..73aac0f 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1459,13 +1459,13 @@ static int img_convert(int argc, char **argv) buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE); if (skip_create) { - int64_t output_length = bdrv_getlength(out_bs); - if (output_length < 0) { + int64_t output_sectors = bdrv_nb_sectors(out_bs); + if (output_sectors < 0) { error_report("unable to get output image length: %s\n", - strerror(-output_length)); + strerror(-output_sectors)); ret = -1; goto out; - } else if (output_length < total_sectors << BDRV_SECTOR_BITS) { + } else if (output_sectors < total_sectors) { error_report("output file is smaller than input file"); ret = -1; goto out;
Instead of bdrv_nb_sectors(). Aside: a few of these callers don't handle errors. I didn't investigate whether they should. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block-migration.c | 9 ++++----- block.c | 40 ++++++++++++++++++---------------------- block/qcow2.c | 2 +- block/vmdk.c | 5 ++--- qemu-img.c | 8 ++++---- 5 files changed, 29 insertions(+), 35 deletions(-)