[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails. |
Date: |
Fri, 03 Jul 2015 14:35:52 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
"Richard W.M. Jones" <address@hidden> writes:
> Currently if qemu is connected to a curl source (eg. web server), and
> the web server fails / times out / dies, you always see a bogus EIO
> "Input/output error".
>
> For example, choose a large file located on any local webserver which
> you control:
>
> $ qemu-img convert -p http://example.com/large.iso /tmp/test
>
> Once it starts copying the file, stop the webserver and you will see
> qemu-img fail with:
>
> qemu-img: error while reading sector 61440: Input/output error
>
> This patch does two things: Firstly print the actual error from curl
> so it doesn't get lost. Secondly, change EIO to EPROTO. EPROTO is a
> POSIX.1 compatible errno which more accurately reflects that there was
> a protocol error, rather than some kind of hardware failure.
>
> After this patch is applied, the error changes to:
>
> $ qemu-img convert -p http://example.com/large.iso /tmp/test
> qemu-img: curl: transfer closed with 469989 bytes remaining to read
> qemu-img: error while reading sector 16384: Protocol error
>
> Signed-off-by: Richard W.M. Jones <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> ---
> block/curl.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/block/curl.c b/block/curl.c
> index 3a2b63e..2fd7c06 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu-common.h"
> +#include "qemu/error-report.h"
> #include "block/block_int.h"
> #include "qapi/qmp/qbool.h"
> #include "qapi/qmp/qstring.h"
> @@ -298,6 +299,12 @@ static void curl_multi_check_completion(BDRVCURLState *s)
> /* ACBs for successful messages get completed in curl_read_cb */
> if (msg->data.result != CURLE_OK) {
> int i;
> +
> + /* Don't lose the original error message from curl, since
> + * it contains extra data.
> + */
> + error_report("curl: %s", state->errmsg);
> +
> for (i = 0; i < CURL_NUM_ACB; i++) {
> CURLAIOCB *acb = state->acb[i];
>
Printing an error message, then returning an error code is problematic.
It works when the caller is going to print its own error message to the
same destination. Callee produces a specific error message devoid of
context, caller produces an unspecific one with hopefully more context.
Better than just one of them. Worse than a single specific error with
context, but that can't be done with just a "return errno code"
interface.
It's kind of wrong when the caller reports its own error somewhere else,
e.g. to a monitor. Still, when barfing extra info to stderr is the best
we can do, it's better than nothing.
It's more wrong when the caller handles the error quietly. I guess
that's never the case here, but I can't be sure without a lot more
sleuthing. Perhaps Kevin or Stefan can judge this immediately.
> @@ -305,7 +312,7 @@ static void curl_multi_check_completion(BDRVCURLState *s)
> continue;
> }
>
> - acb->common.cb(acb->common.opaque, -EIO);
> + acb->common.cb(acb->common.opaque, -EPROTO);
> qemu_aio_unref(acb);
> state->acb[i] = NULL;
> }
To understand impact exactly, we'd need to figure out where the changed
error code gets consumed. However, I don't expect consumers to check
the actual error code. A quick grep for comparisons with EIO or -EIO
finds nothing related to block I/O, except for nbd_trip() checking the
value of nbd_co_receive_request(), and that's unrelated.
- Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails.,
Markus Armbruster <=