Re: [Qemu-devel] [PATCH v3 4/6] nbd/client: Support qemu-img convert from unaligned size

[Eric Blake]

On 3/29/19 10:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2019 7:27, Eric Blake wrote:
>> If an NBD server advertises a size that is not a multiple of a sector,
>> the block layer rounds up that size, even though we set info.size to
>> the exact byte value sent by the server. The block layer then proceeds
>> to let us read or query block status on the hole that it added past
>> EOF, which the NBD server is unlikely to be happy with. Fortunately,
>> qemu as a server never advertizes an unaligned size, so we generally
>> don't run into this problem; but the nbdkit server makes it easy to
>> test:
>>
>> $ printf %1000d 1 > f1
>> $ ~/nbdkit/nbdkit -fv file f1 & pid=$!
>> $ qemu-img convert -f raw nbd://localhost:10809 f2
>> $ kill $pid
>> $ qemu-img compare f1 f2
>>

>> @@ -966,7 +985,8 @@ int coroutine_fn 
>> nbd_client_co_block_status(BlockDriverState *bs,
>>           .from = offset,
>>           .len = MIN(MIN_NON_ZERO(QEMU_ALIGN_DOWN(INT_MAX,
>>                                                   bs->bl.request_alignment),
>> -                                client->info.max_block), bytes),
>> +                                client->info.max_block),
>> +                   MIN(bytes, client->info.size - offset)),

This is a complex computation...

>>           .flags = NBD_CMD_FLAG_REQ_ONE,
>>       };
>>
>> @@ -977,6 +997,23 @@ int coroutine_fn 
>> nbd_client_co_block_status(BlockDriverState *bs,
>>           return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
>>       }
>>
>> +    /*
>> +     * Work around the fact that the block layer doesn't do
>> +     * byte-accurate sizing yet - if the status request exceeds the
>> +     * server's advertised size because the block layer rounded size
>> +     * up, we truncated the request to the server (above), or are
>> +     * called on just the hole.
>> +     */
>> +    if (offset >= client->info.size) {
>> +        *pnum = bytes;
>> +        assert(bytes < BDRV_SECTOR_SIZE);
>> +        /* Intentionally don't report offset_valid for the hole */
>> +        return BDRV_BLOCK_ZERO;
>> +    }
>> +
>> +    if (client->info.min_block) {
>> +        assert(QEMU_IS_ALIGNED(request.len, client->info.min_block));

...so the assert is here to say that before we hand a length to the
server, check that our computation ended up being aligned to the
server's requirements on input.

> 
> it will crash if info.size is unaligned..

It will crash if our client code is buggy, before the server ever gets a
chance to see things. Our client code should not be buggy (we do not
want to be sending the server unaligned requests).

> 
> " If a server advertises a minimum block size, the advertised export size 
> SHOULD be an integer multiple of that block size"
> 
> violation "SHOULD" by server, should it lead to client crash?

We are not preventing the server replying with unaligned data, but the
client sending an unaligned request. You're right that we must not
abort() on anything the server sends (even if it was not compliant). But
the client code is under our control, and I'd rather catch it up front
if I have a bug in my complex calculations rather than waiting for
whatever the server decides to do with my buggy request.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature