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

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

[Richard W.M. Jones]

Since v2:

This adds a generalized anti-flooding mechanism, and then uses it to
ensure that curl errors wouldn't flood the log files.

Rich.

[Qemu-devel] [PATCH v3 1/2] Add a simple mechanism to protect against error message floods.

[Richard W.M. Jones]

You can now write code like this:

  FLOOD_COUNTER(errcount, 100);
  ...
  NO_FLOOD(errcount,
           error_report("oops, something bad happened"),
           error_report("further errors suppressed"));

which would print the "oops, ..." error message up to 100 times,
followed by the "further errors suppressed" message once, and then
nothing else.

Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
---
 include/qemu/no-flood.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 include/qemu/no-flood.h

diff --git a/include/qemu/no-flood.h b/include/qemu/no-flood.h
new file mode 100644
index 0000000..90a24da
--- /dev/null
+++ b/include/qemu/no-flood.h
@@ -0,0 +1,34 @@
+/*
+ * Suppress error floods.
+ *
+ * Copyright (C) 2015 Red Hat, Inc.
+ *
+ * Author: Richard W.M. Jones <rjones@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef __QEMU_NO_FLOOD_H
+#define __QEMU_NO_FLOOD_H 1
+
+#include "qemu/atomic.h"
+
+/* Suggested initial value is 100 */
+#define FLOOD_COUNTER(counter_name, initial) \
+  static int counter_name = (initial)
+
+#define NO_FLOOD(counter_name, code, suppress_message)         \
+  do {                                                         \
+    int t_##counter_name = atomic_fetch_add(&counter_name, 0); \
+    if (t_##counter_name > 0) {                                \
+        code;                                                  \
+        if (t_##counter_name == 1) {                           \
+            suppress_message;                                  \
+        }                                                      \
+        atomic_dec(&counter_name);                             \
+    }                                                          \
+} while (0)
+
+#endif
-- 
2.3.1

[Qemu-devel] [PATCH v3 2/2] 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 | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/curl.c b/block/curl.c
index 3a2b63e..b72d505 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -22,6 +22,8 @@
  * THE SOFTWARE.
  */
 #include "qemu-common.h"
+#include "qemu/error-report.h"
+#include "qemu/no-flood.h"
 #include "block/block_int.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qstring.h"
@@ -298,6 +300,15 @@ 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;
+                FLOOD_COUNTER(errcount, 100);
+
+                /* Don't lose the original error message from curl, since
+                 * it contains extra data.
+                 */
+                NO_FLOOD(errcount,
+                         error_report("curl: %s", state->errmsg),
+                         error_report("curl: further errors suppressed"));
+
                 for (i = 0; i < CURL_NUM_ACB; i++) {
                     CURLAIOCB *acb = state->acb[i];
 
@@ -305,7 +316,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 v3 1/2] Add a simple mechanism to protect against error message floods.

[Kevin Wolf]

Am 08.07.2015 um 14:50 hat Richard W.M. Jones geschrieben:
> You can now write code like this:
> 
>   FLOOD_COUNTER(errcount, 100);
>   ...
>   NO_FLOOD(errcount,
>            error_report("oops, something bad happened"),
>            error_report("further errors suppressed"));
> 
> which would print the "oops, ..." error message up to 100 times,
> followed by the "further errors suppressed" message once, and then
> nothing else.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> ---
>  include/qemu/no-flood.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 include/qemu/no-flood.h
> 
> diff --git a/include/qemu/no-flood.h b/include/qemu/no-flood.h
> new file mode 100644
> index 0000000..90a24da
> --- /dev/null
> +++ b/include/qemu/no-flood.h
> @@ -0,0 +1,34 @@
> +/*
> + * Suppress error floods.
> + *
> + * Copyright (C) 2015 Red Hat, Inc.
> + *
> + * Author: Richard W.M. Jones <rjones@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef __QEMU_NO_FLOOD_H
> +#define __QEMU_NO_FLOOD_H 1
> +
> +#include "qemu/atomic.h"
> +
> +/* Suggested initial value is 100 */
> +#define FLOOD_COUNTER(counter_name, initial) \
> +  static int counter_name = (initial)
> +
> +#define NO_FLOOD(counter_name, code, suppress_message)         \
> +  do {                                                         \
> +    int t_##counter_name = atomic_fetch_add(&counter_name, 0); \
> +    if (t_##counter_name > 0) {                                \
> +        code;                                                  \
> +        if (t_##counter_name == 1) {                           \
> +            suppress_message;                                  \
> +        }                                                      \
> +        atomic_dec(&counter_name);                             \
> +    }                                                          \
> +} while (0)
> +
> +#endif

I don't think that you actually achieve thread safety with the atomic
operations you use here. The counter may change between your check and
the atomic_dec().

It doesn't matter for curl because we don't get concurrency there
anyway, but it might confuse others if it looks as if it was thread safe
when it fact it isn't.

So I'd suggest that if you have an easy fix for thread safety, do it,
but otherwise don't bother and just remove the atomics.

Other than that, I like this series.

Kevin

Re: [Qemu-devel] [PATCH v3 1/2] Add a simple mechanism to protect against error message floods.

[Paolo Bonzini]

On 08/07/2015 15:26, Kevin Wolf wrote:
>> +/* Suggested initial value is 100 */
>> +#define FLOOD_COUNTER(counter_name, initial) \
>> +  static int counter_name = (initial)

No "static" here.

>> +#define NO_FLOOD(counter_name, code, suppress_message)         \
>> +  do {                                                         \
>> +    int t_##counter_name = atomic_fetch_add(&counter_name, 0); \
>> +    if (t_##counter_name > 0) {                                \
>> +        code;                                                  \
>> +        if (t_##counter_name == 1) {                           \
>> +            suppress_message;                                  \
>> +        }                                                      \
>> +        atomic_dec(&counter_name);                             \
>> +    }                                                          \
>> +} while (0)

What you want is a cmpxchg loop like:

   int old;
   do
       old = atomic_read(&counter_name);
   while (old > 0 && atomic_cmpxchg(&counter_name, old, old - 1) != old);
   if (old > 0) {
       code;
       if (old == 1) {
           suppress_message;
       }
   }

but I would wrap it with a simple API like

   void error_report_limited(int *counter, const char *s, ...);
   void error_report_err_limited(int *counter, Error *err);

instead of your complex NO_FLOOD macro.

Paolo