[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report() |
Date: |
Mon, 26 May 2014 17:16:24 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 26.05.2014 um 17:02 hat Markus Armbruster geschrieben:
> Stefan Hajnoczi <address@hidden> writes:
>
> > On Mon, May 26, 2014 at 09:44:03AM +0800, Le Tan wrote:
> >> Replace fprintf(stderr,...) with error_report() in files block/*, block.c,
> >> block-migration.c and blockdev.c. The trailing "\n"s of the @fmt argument
> >> have been removed because @fmt of error_report() should not contain
> >> newline.
> >>
> >> Signed-off-by: Le Tan <address@hidden>
> >> ---
> >> block-migration.c | 6 +--
> >> block.c | 4 +-
> >> block/qcow2-refcount.c | 115 +++++++++++++++++++++---------------------
> >> block/qcow2.c | 16 +++---
> >> block/raw-posix.c | 8 ++-
> >> block/raw-win32.c | 6 +--
> >> block/ssh.c | 2 +-
> >> block/vdi.c | 14 +++---
> >> block/vmdk.c | 15 +++---
> >> block/vpc.c | 4 +-
> >> block/vvfat.c | 129
> >> ++++++++++++++++++++++++------------------------
> >> blockdev.c | 6 +--
> >> 12 files changed, 159 insertions(+), 166 deletions(-)
> >
> > There is one thing that worries me about this:
> >
> > error_report() assumes that the QEMU global mutex is held in order to
> > access the "current monitor".
>
> Global variable cur_mon, non-null while we're executing a monitor
> command.
The important part here is that it's indeed global and not thread-local.
> > This is problematic for code in the read/write/flush path (like qcow2
> > refcount allocation) since it can be invoked from a virtio-blk
> > data-plane thread (where the QEMU global mutex is not held).
>
> error_report() reports to the current monitor when "in monitor context",
> i.e. while executing a monitor command, i.e. while cur_mon isn't null.
>
> Can we ever be in monitor context (cur_mon not null) without holding the
> global mutex?
The right question is: Can a thread (= the main loop thread) ever be in
monitor context while another thread (= dataplane thread) is executing
block driver code and doesn't hold the global mutex?
If I understand dataplane correctly, the whole point of it is that the
answer to this is yes.
Kevin
> > For the time being, I prefer keeping the fprintf(). In the long term we
> > need to decide:
> > 1. Some functions should use Error * to report errors.
> > 2. Some functions really produce their own output but it probably
> > shouldn't go to the monitor.
> > 3. The remaining functions may really be error_report() candidates.
>
> 0. Figure out what rules apply to cur_mon with respect to the global
> mutex.
- [Qemu-trivial] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Le Tan, 2014/05/25
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Stefan Hajnoczi, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Markus Armbruster, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(),
Kevin Wolf <=
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Markus Armbruster, 2014/05/26
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Stefan Hajnoczi, 2014/05/27
- Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] block: replace fprintf(stderr, ...) with error_report(), Markus Armbruster, 2014/05/27