Re: [Qemu-devel] [PATCH 17/18] nbd: BLOCK_STATUS for standard get_block_status function: server part

[Paolo Bonzini]

On 09/02/2017 16:38, Eric Blake wrote:
>> +static int blockstatus_to_extent(BlockDriverState *bs, uint64_t offset,
>> +                                  uint64_t length, NBDExtent *extent)
>> +{
>> +    BlockDriverState *file;
>> +    uint64_t start_sector = offset >> BDRV_SECTOR_BITS;
>> +    uint64_t last_sector = (offset + length - 1) >> BDRV_SECTOR_BITS;
> Converting from bytes to sectors by rounding...
> 
>> +    uint64_t begin = start_sector;
>> +    uint64_t end = last_sector + 1;
>> +
>> +    int nb = MIN(INT_MAX, end - begin);
>> +    int64_t ret = bdrv_get_block_status_above(bs, NULL, begin, nb, &nb, 
>> &file);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    extent->flags =
>> +        cpu_to_be32((ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
>> +                    (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0));
>> +    extent->length = cpu_to_be32((nb << BDRV_SECTOR_BITS) -
>> +                                 (offset - (start_sector << 
>> BDRV_SECTOR_BITS)));
> ...then computing the length by undoing the rounding. I really think we
> should consider fixing bdrv_get_block_status_above() to be byte-based,
> but that's a separate series.  Your calculations look correct in the
> meantime, although '(offset & (BDRV_SECTOR_SIZE - 1))' may be a bit
> easier to read than '(offset - (start_sector << BDRV_SECTOR_BITS))'.

Agreed.  And please make it a separate variable, i.e.

    uint64_t length;

    length = (nb << BDRV_SECTOR_BITS) - (offset & BDRV_SECTOR_SIZE - 1);
    ...
    extent->length = cpu_to_be32(length);

Paolo

Paolo

signature.asc
Description: OpenPGP digital signature