Re: migration/rdma.c's macro ERROR()

[Peter Xu <peterx@redhat.com>]

On Fri, May 05, 2023 at 12:51:45PM +0200, Markus Armbruster wrote:
> migration/rdma.c has
> 
>     /*
>      * Print and error on both the Monitor and the Log file.
>      */
>     #define ERROR(errp, fmt, ...) \
>         do { \
>             fprintf(stderr, "RDMA ERROR: " fmt "\n", ## __VA_ARGS__); \
>             if (errp && (*(errp) == NULL)) { \
>                 error_setg(errp, "RDMA ERROR: " fmt, ## __VA_ARGS__); \
>             } \
>         } while (0)
> 
> This is problematic.  The point of error_setg() & friends is detectin
> errors from handling them.  error.h:
> 
>  * - Separation of concerns: the function is responsible for detecting
>  *   errors and failing cleanly; handling the error is its caller's
>  *   job.  [...]
> 
> Reporting the error to stderr violates this principle.  Consequences
> include
> 
> * When the caller reports the error to stderr, it gets reported there
>   twice, possibly in slightly different form.
> 
> * When the caller recovers from the error cleanly without reporting it,
>   it is reported to stderr anyway, even though it is not actually an
>   error.
> 
> Mind if I kill the macro?

Makes sense to me.

/me copies the most recent (in past two years) active developers on rdma so
they're aware.

-- 
Peter Xu