Re: [Qemu-devel] [PATCH v1 0/8] Remove some of the fprintf(stderr, "*

From: Stefan Hajnoczi <stefanha@gmail.com>
To: Alistair Francis <alistair.francis@xilinx.com>
Cc: qemu-devel@nongnu.org, alistair23@gmail.com, armbru@redhat.com,
	Kevin Wolf <kwolf@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v1 0/8]  Remove some of the fprintf(stderr, "*
Date: Tue, 26 Sep 2017 10:05:21 +0100	[thread overview]
Message-ID: <20170926090521.GE16834@stefanha-x1.localdomain> (raw)
In-Reply-To: <cover.1506384414.git.alistair.francis@xilinx.com>

On Mon, Sep 25, 2017 at 05:08:34PM -0700, Alistair Francis wrote:
> 
> Continue on improving QEMUs logging/error messages by removing more
> fprintf()'s.
> 
> Unfortunatley my Coccinelle skills aren't that great so it's all done in
> some nasty regex and a little bit of manual work.
> 
> 
> 
> Alistair Francis (8):
>   Replace all occurances of __FUNCTION__ with __func__
>   tests: Replace fprintf(stderr, "*\n" with error_report()
>   hw: Replace fprintf(stderr, "*\n" with error_report()
>   block: Replace fprintf(stderr, "*\n" with error_report()
>   util: Replace fprintf(stderr, "*\n" with error_report()
>   ui: Replace fprintf(stderr, "*\n" with error_report()
>   tcg: Replace fprintf(stderr, "*\n" with error_report()
>   target: Replace fprintf(stderr, "*\n" with error_report()
> 
>  audio/audio_int.h                      |   2 +-
>  block/file-posix.c                     |   6 +-
>  block/file-win32.c                     |   3 +-
>  block/linux-aio.c                      |   5 +-
>  block/parallels.c                      |   7 ++-
>  block/qcow2-cluster.c                  |   2 +-
>  block/qcow2-refcount.c                 |  95 +++++++++++++++----------------
>  block/qcow2.c                          |   8 +--
>  block/quorum.c                         |   5 +-
>  block/ssh.c                            |   4 +-
>  block/vdi.c                            |  14 +++--
>  block/vpc.c                            |   5 +-
>  block/vvfat.c                          |  99 ++++++++++++++++++---------------

Please don't convert fprintfs to error_report in the block layer yet.
error_report() uses the cur_mon global variable in a way that is not
thread-safe:

  void error_vprintf(const char *fmt, va_list ap)
  {
      if (cur_mon && !monitor_cur_is_qmp()) {
          monitor_vprintf(cur_mon, fmt, ap);

This is a time-of-check-to-time-of-use race condition.
monitor_cur_is_qmp() itself also suffers from a race condition.

I guess we haven't seen issues because error_report() is rarely called.

Stefan