Re: [RFC PATCH] curl: Allow reading after EOF

[Eric Blake]

On 3/17/21 10:32 AM, Eric Blake wrote:
> On 3/17/21 10:17 AM, Kevin Wolf wrote:
>> This makes the curl driver more consistent with file-posix in that it
>> doesn't return errors any more for reading after the end of the remote
>> file. Instead, zeros are returned for these areas.
>>
>> This inconsistency was reported in:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1935061
>>
>> Note that the image used in this bug report has a corrupted snapshot
>> table, which means that the qcow2 driver tries to do a zero-length read
>> after EOF on its image file.
>>
>> The old behaviour of the curl driver can hardly be called a bug, but the
>> inconsistency turned out to be confusing.
>>
>> Reported-by: Alice Frosi <afrosi@redhat.com>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>
>> It is not entirely clear to me if this is something we want to do. If we
>> do care about consistency between protocol drivers, something like this
>> should probably be done in block/io.c eventually - but that would
>> require converting bs->total_sectors to byte granularity first.
> 
> Something that's been (low priority) on my todo list for a while.  NBD
> has the same problem.

Actually, NBD has already been patched to fuzz around the lack of
byte-accurateness in the block layer; see commit 9cf638508.  So doing
something similar in the curl driver as a workaround until the block
layer does it for everyone is tolerable, but does not scale.

> 
>>
>> Any opinions on what the most desirable semantics would be and whether
>> we should patch individual drivers until we can have a generic solution?
> 
> In nbdkit, we took the following approach in the 'truncate' driver:
> 
> If presented with an image that is not a multiple of the desired block
> size, we round the image size up (corner cases for images with sizes
> near 2^63 where rounding would wrap to negative; and since qemu enforces
> a max image size at 2^63-2^32 to avoid 32-bit operations ever
> overflowing).  Reads of the virtual tail come back as zero, writes to
> the virtual tail are allowed if they would write zero into the tail, and
> fail with ENOSPC otherwise.

The current code in block/nbd.c does this for reads, but fails on EIO
without regards to the content of what is being attempted to write into
that tail.  I like the nbdkit behavior better.

> 
> Doing that in the block layer makes more sense than doing it per-driver.
> 
> Thus, I'm not sure if I'm a fan of this patch.
> 
>>
>>  block/curl.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/block/curl.c b/block/curl.c
>> index 50e741a0d7..a8d87a1813 100644
>> --- a/block/curl.c
>> +++ b/block/curl.c
>> @@ -898,6 +898,7 @@ out:
>>  static int coroutine_fn curl_co_preadv(BlockDriverState *bs,
>>          uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
>>  {
>> +    BDRVCURLState *s = bs->opaque;
>>      CURLAIOCB acb = {
>>          .co = qemu_coroutine_self(),
>>          .ret = -EINPROGRESS,
>> @@ -906,6 +907,15 @@ static int coroutine_fn curl_co_preadv(BlockDriverState 
>> *bs,
>>          .bytes = bytes
>>      };
>>  
>> +    if (offset > s->len || bytes > s->len - offset) {
>> +        uint64_t req_bytes = offset > s->len ? 0 : s->len - offset;
>> +        qemu_iovec_memset(qiov, req_bytes, 0, bytes - req_bytes);
>> +        bytes = req_bytes;

In nbd.c, I also have:
   if (offset >= client->info.size) {
        assert(bytes < BDRV_SECTOR_SIZE);

    if (offset + bytes > client->info.size) {
        assert(slop < BDRV_SECTOR_SIZE);

With those assertions added, I can give it

Reviewed-by: Eric Blake <eblake@redhat.com>

>> +    }
>> +    if (bytes == 0) {
>> +        return 0;
>> +    }
>> +
>>      curl_setup_preadv(bs, &acb);
>>      while (acb.ret == -EINPROGRESS) {
>>          qemu_coroutine_yield();
>>
> 

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