[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_bloc
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status() |
Date: |
Wed, 31 May 2017 16:53:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 |
On 2017-05-25 08:34, Fam Zheng wrote:
> On Wed, 05/24 15:28, Eric Blake wrote:
>> Not all callers care about which BDS owns the mapping for a given
>> range of the file. This patch merely simplifies the callers by
>> consolidating the logic in the common call point, while guaranteeing
>> a non-NULL file to all the driver callbacks, for no semantic change.
>>
>> However, this will also set the stage for a future cleanup: when a
>> caller does not care about which BDS owns an offset, it would be
>> nice to allow the driver to optimize things to not have to return
>> BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented
>> allocation (for example, it's fairly easy to create a qcow2 image
>> where consecutive guest addresses are not at consecutive host
>> addresses), the current contract requires bdrv_get_block_status()
>> to clamp *pnum to the limit where host addresses are no longer
>> consecutive, but allowing a NULL file means that *pnum could be
>> set to the full length of known-allocated data.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> ---
>> v2: new patch
>
> Yes. any particular reason why this patch is useful, besides simplifying
> callers?
>
>
>> ---
>> block/io.c | 15 +++++++++------
>> block/mirror.c | 3 +--
>> block/qcow2.c | 4 +---
>> qemu-img.c | 10 ++++------
>> 4 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 8e6c3fe..eea74cb 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags
>> flags)
>> {
>> int64_t target_sectors, ret, nb_sectors, sector_num = 0;
>> BlockDriverState *bs = child->bs;
>> - BlockDriverState *file;
>> int n;
>>
>> target_sectors = bdrv_nb_sectors(bs);
>> @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags
>> flags)
>> if (nb_sectors <= 0) {
>> return 0;
>> }
>> - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file);
>> + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL);
>> if (ret < 0) {
>> error_report("error getting block status at sector %" PRId64 ":
>> %s",
>> sector_num, strerror(-ret));
>> @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData {
>> * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors
>> goes
>> * beyond the end of the disk image it will be clamped.
>> *
>> - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set,
>> 'file'
>> - * points to the BDS which the sector range is allocated in.
>> + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and
>> + * 'file' is non-NULL, then '*file' points to the BDS which the sector range
>> + * is allocated in.
>
> Sounds good.
>
>> */
>> static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
>> int64_t sector_num,
>> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn
>> bdrv_co_get_block_status(BlockDriverState *bs,
>> int64_t total_sectors;
>> int64_t n;
>> int64_t ret, ret2;
>> + BlockDriverState *tmpfile;
>>
>> + if (!file) {
>> + file = &tmpfile;
>> + }
>
> I don't like this hunk. Instead, how about replacing all "*file = ..." with
> "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning?
Sounds fine to me, but in that case I'd like to request a different
variable name. Maybe do what we have with errp/local_error and make it
local_file or something?
Max
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v2 0/5] more blkdebug tweaks, Eric Blake, 2017/05/24
- [Qemu-block] [PATCH v2 1/5] qemu-io: Don't die on second open, Eric Blake, 2017/05/24
- [Qemu-block] [PATCH v2 2/5] block: Guarantee that *file is set on bdrv_get_block_status(), Eric Blake, 2017/05/24
- [Qemu-block] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status(), Eric Blake, 2017/05/24
- [Qemu-block] [PATCH v2 4/5] block: Simplify use of BDRV_BLOCK_RAW, Eric Blake, 2017/05/24
- [Qemu-block] [PATCH v2 5/5] blkdebug: Support .bdrv_co_get_block_status, Eric Blake, 2017/05/24
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 0/5] more blkdebug tweaks, Fam Zheng, 2017/05/25