[v3,1/2] Add a simple mechanism to protect against error message floods.

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

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

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