On Wed, Mar 17, 2021 at 04:17:34PM +0100, 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 <afr...@redhat.com>
> Signed-off-by: Kevin Wolf <kw...@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.
>
> Any opinions on what the most desirable semantics would be and whether
> we should patch individual drivers until we can have a generic solution?
What valid scenarios are there for wanting to read beyond the bounds
of the protocol driver storage ? Why was file-posix allowing this
so far ?
If I've given file-posix a 10 GB plain file or device and something
requests a read from the 11 GB offset, IMHO, that is a sign of serious
error somewhere and possible impending doom.
For writable storage, I would think that read + write should be
symmetric, by which I mean if a read() at a particular offset
succeeds, then I would also expect a write() at the same offset to
succeed, and have its data later returned by a read().
We generally can't write at an offset beyond the storage (unless we
are intending to auto-enlarge a plain file), so I think we shouldn't
allow reads either.
>
> 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();
> --
> 2.30.2
>
>
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
