[Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection

[Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection

[Richard W.M. Jones]

No change since version 1, but I've added Jeff and Kevin to the
CC as suggested by 'scripts/get_maintainer.pl'.

[Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails.

[Richard W.M. Jones]

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 <rjones@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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];
 
@@ -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;
                 }
-- 
2.3.1

Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails.

[Markus Armbruster]

"Richard W.M. Jones" <rjones@redhat.com> 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 <rjones@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  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.

[Kevin Wolf]

Am 03.07.2015 um 14:35 hat Markus Armbruster geschrieben:
> "Richard W.M. Jones" <rjones@redhat.com> 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 <rjones@redhat.com>
> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  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.

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

Yes, I wouldn't expect any problems caused by this change.

Kevin

Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails.

[Richard W.M. Jones]

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" <rjones@redhat.com> 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 <rjones@redhat.com>
> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > >  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

Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails.

[Kevin Wolf]

Am 08.07.2015 um 13:36 hat Richard W.M. Jones geschrieben:
> 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" <rjones@redhat.com> 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 <rjones@redhat.com>
> > > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > ---
> > > >  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.

The guest can't cause it, but once the connection is down, I expect
every request to fail. You don't have to have a malicious guest for
filling up the log file, it just needs to be careless enough to continue
trying new requests instead of offlining the device.

Kevin

Re: [Qemu-devel] [PATCH v2] block/curl: Don't lose original error when a connection fails.

[Richard W.M. Jones]

On Wed, Jul 08, 2015 at 02:01:30PM +0200, Kevin Wolf wrote:
> The guest can't cause it, but once the connection is down, I expect
> every request to fail. You don't have to have a malicious guest for
> filling up the log file, it just needs to be careless enough to continue
> trying new requests instead of offlining the device.

How about something along these lines?

[I'm not clear if atomic is necessary here, nor if there is already
some mechanism for suppressing log messages - a cursory grep of the
qemu source didn't find anything.]

diff --git a/block/curl.c b/block/curl.c
index 2fd7c06..33c14d8 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -299,11 +299,20 @@ 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;
+                static int errcount = 100;
+                int current_errcount;
 
                 /* Don't lose the original error message from curl, since
                  * it contains extra data.
                  */
-                error_report("curl: %s", state->errmsg);
+                current_errcount = atomic_fetch_add(&errcount, 0);
+                if (current_errcount > 0) {
+                    error_report("curl: %s", state->errmsg);
+                    if (current_errcount == 1) {
+                        error_report("curl: further errors suppressed");
+                    }
+                    atomic_dec(&errcount);
+                }
 
                 for (i = 0; i < CURL_NUM_ACB; i++) {
                     CURLAIOCB *acb = state->acb[i];

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org