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

[Kevin Wolf <kwolf@redhat.com>]

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