[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/5] block: Avoid bdrv_get_geometry() where erro
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 5/5] block: Avoid bdrv_get_geometry() where errors should be detected |
Date: |
Wed, 28 May 2014 13:48:55 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> 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 <address@hidden>
>
>> @@ -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.
Before my patch:
1535 while (bs_num == bs_sectors) {
1536 bs_i++;
1537 assert (bs_i < bs_n);
1538 bs_offset += bs_sectors;
1539 bdrv_get_geometry(bs[bs_i], &bs_sectors);
1540 bs_num = 0;
1541 /* printf("changing part: sector_num=%" PRId64 ", "
1542 "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64
1543 "\n", sector_num, bs_i, bs_offset, bs_sectors); */
1544 }
Before the loop, bs_i is 0, and bs_sectors is the number of sectors in
bs[0] (see previous hunk).
In the i-th iteration (counting from 1):
1538 use number of sectors in bs[i-1]
1539 compute number of sectors in bs[i]
1544 use number of sectors in bs[i]
After my patch:
1548 while (bs_num == bs_sectors[bs_i]) {
1549 bs_i++;
1550 assert (bs_i < bs_n);
1551 bs_offset += bs_sectors[bs_i];
1552 bs_num = 0;
1553 /* printf("changing part: sector_num=%" PRId64 ", "
1554 "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64
1555 "\n", sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */
1556 }
You're right. I'll fix it to look like the other loop, where I got the
order right.
>> 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.
The main objective is to get rid of bdrv_get_geometry() without having
to recheck errors again and again :)
But point taken. I'll split this up.