[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected |
Date: |
Wed, 04 Jun 2014 15:20:18 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Benoît Canet <address@hidden> writes:
> The Wednesday 04 Jun 2014 à 13:51:51 (+0200), Markus Armbruster wrote :
>> 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>
>> Reviewed-by: Eric Blake <address@hidden>
>> Reviewed-by: Max Reitz <address@hidden>
[...]
>> diff --git a/qemu-img.c b/qemu-img.c
>> index e6d0edf..7e6dde0 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv)
>> int64_t sector_num = 0;
>> int64_t nb_sectors;
>> int c, pnum;
>> - uint64_t bs_sectors;
>> uint64_t progress_base;
>>
>> for (;;) {
>> @@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv)
>>
>> buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>> buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
>> - bdrv_get_geometry(bs1, &bs_sectors);
>> - total_sectors1 = bs_sectors;
>> - bdrv_get_geometry(bs2, &bs_sectors);
>> - total_sectors2 = bs_sectors;
>> + total_sectors1 = bdrv_nb_sectors(bs1);
>> + if (total_sectors1 < 0) {
>> + error_report("Can't get size of %s: %s",
>> + filename1, strerror(-total_sectors1));
>> + ret = 4;
>> + goto out;
>> + }
>> + total_sectors2 = bdrv_nb_sectors(bs2);
>> + if (total_sectors2 < 0) {
>> + error_report("Can't get size of %s: %s",
>> + filename2, strerror(-total_sectors2));
>> + ret = 4;
>> + goto out;
>> + }
>> total_sectors = MIN(total_sectors1, total_sectors2);
>> progress_base = MAX(total_sectors1, total_sectors2);
>>
>> @@ -1185,7 +1194,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 = NULL;
>> + int64_t *bs_sectors = NULL;
>> uint8_t * buf = NULL;
>> size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>> const uint8_t *buf1;
>> @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
>> qemu_progress_print(0, 100);
>>
>> bs = g_new0(BlockDriverState *, bs_n);
>> - bs_sectors = g_new(uint64_t, bs_n);
>> + bs_sectors = g_new(int64_t, bs_n);
>>
>> total_sectors = 0;
>> for (bs_i = 0; bs_i < bs_n; bs_i++) {
>> @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
>> ret = -1;
>> goto out;
>> }
>> - bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
>> + 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];
>> }
>>
>> @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
>> * the image is the same as the original one at any time.
>> */
>> if (!unsafe) {
>> - uint64_t num_sectors;
>> - uint64_t old_backing_num_sectors;
>> - uint64_t new_backing_num_sectors = 0;
>> + int64_t num_sectors;
>> + int64_t old_backing_num_sectors;
>> + int64_t new_backing_num_sectors = 0;
>> uint64_t sector;
>> int n;
>> uint8_t * buf_old;
>> @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
>> buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
>> buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
>>
>> - bdrv_get_geometry(bs, &num_sectors);
>> - bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
>> + num_sectors = bdrv_nb_sectors(bs);
>> + if (num_sectors < 0) {
>> + error_report("Could not get size of '%s': %s",
>> + filename, strerror(-num_sectors));
>> + ret = -1;
>> + goto out;
>> + }
>> + old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
>> + if (old_backing_num_sectors < 0) {
>> + char backing_name[1024];
>
> Could you put this on the heap ?
>
> I recently fixed a stack overflow when taking snapshots due to multiple
> PATH_MAX
> char array in a recursive function.
>
> We don't know how this function will be used later.
img_rebase() is not a general purpose function, it's a qemu-img command.
Stack use is well below a single page even with my patch. I can't see
how it could possibly become recursive.
- [Qemu-devel] [PATCH v4 05/10] block: Use bdrv_nb_sectors() in bdrv_co_get_block_status(), (continued)
- [Qemu-devel] [PATCH v4 05/10] block: Use bdrv_nb_sectors() in bdrv_co_get_block_status(), Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 02/10] block: New bdrv_nb_sectors(), Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 06/10] block: Use bdrv_nb_sectors() in img_convert(), Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 08/10] block: Drop superfluous aligning of bdrv_getlength()'s value, Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 09/10] qemu-img: Make img_convert() get image size just once per image, Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 01/10] raw-posix: Fix raw_getlength() to always return -errno on error, Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 07/10] block: Use bdrv_nb_sectors() where sectors, not bytes are wanted, Markus Armbruster, 2014/06/04
- [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected, Markus Armbruster, 2014/06/04
- Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected, Benoît Canet, 2014/06/04