Am 09.05.2014 um 11:48 hat Markus Armbruster geschrieben:
> bdrv_get_geometry() hides errors. Use bdrv_nb_sectors() or
> bdrv_getlength() instead where that's obviously inappropriate.
>
> Signed-off-by: Markus Armbruster <arm...@redhat.com>
> @@ -1167,7 +1174,7 @@ static int img_convert(int argc, char **argv)
> BlockDriver *drv, *proto_drv;
> BlockDriverState **bs = NULL, *out_bs = NULL;
> int64_t total_sectors, nb_sectors, sector_num, bs_offset;
> - uint64_t bs_sectors;
> + int64_t *bs_sectors = NULL;
> uint8_t * buf = NULL;
> size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
> const uint8_t *buf1;
> @@ -1307,7 +1314,8 @@ static int img_convert(int argc, char **argv)
>
> qemu_progress_print(0, 100);
>
> - bs = g_malloc0(bs_n * sizeof(BlockDriverState *));
> + bs = g_new(BlockDriverState *, bs_n);
> + bs_sectors = g_new(int64_t, bs_n);
>
> total_sectors = 0;
> for (bs_i = 0; bs_i < bs_n; bs_i++) {
> @@ -1321,8 +1329,14 @@ static int img_convert(int argc, char **argv)
> ret = -1;
> goto out;
> }
> - bdrv_get_geometry(bs[bs_i], &bs_sectors);
> - total_sectors += bs_sectors;
> + bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
> + if (bs_sectors[bs_i] < 0) {
> + error_report("Could not get size of %s: %s",
> + argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
> + ret = -1;
> + goto out;
> + }
> + total_sectors += bs_sectors[bs_i];
> }
>
> if (sn_opts) {
> @@ -1446,7 +1460,6 @@ static int img_convert(int argc, char **argv)
>
> bs_i = 0;
> bs_offset = 0;
> - bdrv_get_geometry(bs[0], &bs_sectors);
>
> /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
> * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
> @@ -1512,19 +1525,19 @@ static int img_convert(int argc, char **argv)
> buf2 = buf;
> while (remainder > 0) {
> int nlow;
> - while (bs_num == bs_sectors) {
> + while (bs_num == bs_sectors[bs_i]) {
> bs_i++;
> assert (bs_i < bs_n);
> - bs_offset += bs_sectors;
> - bdrv_get_geometry(bs[bs_i], &bs_sectors);
> + bs_offset += bs_sectors[bs_i];
bs_i is already incremented here, so you're accessing the new bs_sectors
instead of the old one.
> bs_num = 0;
> /* printf("changing part: sector_num=%" PRId64 ", "
> "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64
> - "\n", sector_num, bs_i, bs_offset, bs_sectors); */
> + "\n", sector_num, bs_i, bs_offset, bs_sectors[bs_i]);
> */
> }
> - assert (bs_num < bs_sectors);
> + assert (bs_num < bs_sectors[bs_i]);
>
> - nlow = (remainder > bs_sectors - bs_num) ? bs_sectors -
> bs_num : remainder;
> + nlow = remainder > bs_sectors[bs_i] - bs_num
> + ? bs_sectors[bs_i] - bs_num : remainder;
>
> ret = bdrv_read(bs[bs_i], bs_num, buf2, nlow);
> if (ret < 0) {
> @@ -1585,14 +1598,13 @@ restart:
> break;
> }
>
> - while (sector_num - bs_offset >= bs_sectors) {
> + while (sector_num - bs_offset >= bs_sectors[bs_i]) {
> + bs_offset += bs_sectors[bs_i];
> bs_i ++;
Here you got the order right.
> assert (bs_i < bs_n);
> - bs_offset += bs_sectors;
> - bdrv_get_geometry(bs[bs_i], &bs_sectors);
> /* printf("changing part: sector_num=%" PRId64 ", bs_i=%d, "
> "bs_offset=%" PRId64 ", bs_sectors=%" PRId64 "\n",
> - sector_num, bs_i, bs_offset, bs_sectors); */
> + sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */
> }
>
> if ((out_baseimg || has_zero_init) &&
> @@ -1645,7 +1657,7 @@ restart:
> }
> }
>
> - n = MIN(n, bs_sectors - (sector_num - bs_offset));
> + n = MIN(n, bs_sectors[bs_i] - (sector_num - bs_offset));
>
> sectors_read += n;
> if (count_allocated_sectors) {
> @@ -1703,6 +1715,7 @@ out:
> }
> g_free(bs);
> }
> + g_free(bs_sectors);
> fail_getopt:
> g_free(options);
This is not a trivial change that can be done while you're touching the
code for fixing the error paths. The error checking is only a small part
of it, while the main objective seems to be caching image sizes so that
bdrv_getlength() needs to be called only once. I'm not sure if this
optimisation is worthwhile, but if so, it should be a separate patch.
Kevin
