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

[Eric Blake <eblake@redhat.com>]

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.

> 
> 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.

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;
> +    }
> +    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