Re: [PATCH v6 02/12] monitor: Use getter/setter functions for cur_mon

[Kevin Wolf <kwolf@redhat.com>]

Am 28.05.2020 um 20:31 hat Eric Blake geschrieben:
> On 5/28/20 10:37 AM, Kevin Wolf wrote:
> > cur_mon really needs to be coroutine-local as soon as we move monitor
> > command handlers to coroutines and let them yield. As a first step, just
> > remove all direct accesses to cur_mon so that we can implement this in
> > the getter function later.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> 
> > +++ b/audio/wavcapture.c
> > @@ -1,5 +1,5 @@
> >   #include "qemu/osdep.h"
> > -#include "monitor/monitor.h"
> > +#include "qemu/qemu-print.h"
> >   #include "qapi/error.h"
> >   #include "qemu/error-report.h"
> >   #include "audio.h"
> > @@ -94,9 +94,9 @@ static void wav_capture_info (void *opaque)
> >       WAVState *wav = opaque;
> >       char *path = wav->path;
> > -    monitor_printf (cur_mon, "Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> > -                    wav->freq, wav->bits, wav->nchannels,
> > -                    path ? path : "<not available>", wav->bytes);
> > +    qemu_printf("Capturing audio(%d,%d,%d) to %s: %d bytes\n",
> > +                wav->freq, wav->bits, wav->nchannels,
> > +                path ? path : "<not available>", wav->bytes);
> 
> Why the change to qemu_printf() here instead of a call to
> monitor_cur() as is done in the rest of the patch?

There was actually only one other monitor_printf() call for cur_mon, and
that was in the monitor core, so I think there's no reason to switch
that one.

If you prefer, I can call monitor_cur() directly here, but I thought not
calling it from everywhere when there is already a wrapper like
qemu_printf() would be a good thing.

Maybe wav_capture_info() should actually be passed a Monitor object so
we don't need either.

> > +++ b/stubs/monitor-core.c
> > @@ -3,7 +3,10 @@
> >   #include "qemu-common.h"
> >   #include "qapi/qapi-emit-events.h"
> > -__thread Monitor *cur_mon;
> > +Monitor *monitor_cur(void)
> > +{
> > +    return NULL;
> > +}
> 
> monitor_set_cur() didn't need a stub?  Odd, but I guess it's okay.

Not that odd, only the code to handle monitor commands ever sets the
current monitor. But if you use the stubs, you don't have a monitor.

Kevin