[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/curl: Don't lose original
From: |
Richard W.M. Jones |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails. |
Date: |
Wed, 8 Jul 2015 12:36:01 +0100 |
User-agent: |
Mutt/1.5.20 (2009-12-10) |
On Wed, Jul 08, 2015 at 12:23:37PM +0200, Kevin Wolf wrote:
> Am 03.07.2015 um 14:35 hat Markus Armbruster geschrieben:
> > "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.
>
> I'm not worried too much about requests made by the monitor or during
> startup. I don't like the error_report() there, but having a more
> specific error message on stderr is better than having nothing.
>
> The case that bothers me more is guest requests. Depending on the
> werror/rerror settings, this may allow the guest to flood the log file
> with curl error messages.
Can you expand a bit on how they would do this? I can see how the
remote web server can cause a curl error (itself possibly a concern),
but not how the guest can do it.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top