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

[Kevin Wolf]

Am 04.08.2020 um 14:46 hat Markus Armbruster geschrieben:
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index d598dd02bb..f609fcf75b 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1301,11 +1301,11 @@ cleanup:
> >  static void monitor_read(void *opaque, const uint8_t *buf, int size)
> >  {
> >      MonitorHMP *mon;
> > -    Monitor *old_mon = cur_mon;
> > +    Monitor *old_mon = monitor_cur();
> >      int i;
> >  
> > -    cur_mon = opaque;
> > -    mon = container_of(cur_mon, MonitorHMP, common);
> > +    monitor_set_cur(opaque);
> > +    mon = container_of(monitor_cur(), MonitorHMP, common);
> 
> Simpler:
> 
>        MonitorHMP *mon = container_of(opaque, MonitorHMP, common);

opaque is void*, so it doesn't have a field 'common'.

> > diff --git a/monitor/monitor.c b/monitor/monitor.c
> > index 125494410a..182ba136b4 100644
> > --- a/monitor/monitor.c
> > +++ b/monitor/monitor.c
> > @@ -66,13 +66,24 @@ MonitorList mon_list;
> >  int mon_refcount;
> >  static bool monitor_destroyed;
> >  
> > -__thread Monitor *cur_mon;
> > +static __thread Monitor *cur_monitor;
> > +
> > +Monitor *monitor_cur(void)
> > +{
> > +    return cur_monitor;
> > +}
> > +
> > +void monitor_set_cur(Monitor *mon)
> > +{
> > +    cur_monitor = mon;
> > +}
> 
> All uses of monitor_set_cur() look like this:
> 
>     old_mon = monitor_cur();
>     monitor_set_cur(new_mon);
>     ...
>     monitor_set_cur(old_mon);
> 
> If we let monitor_set_cur() return the old value, this becomes
> 
>     old_mon = monitor_set_cur(new_mon);
>     ...
>     monitor_set_cur(old_mon);
> 
> I like this better.

Fine with me.

> > diff --git a/stubs/monitor-core.c b/stubs/monitor-core.c
> > index 6cff1c4e1d..0cd2d864b2 100644
> > --- a/stubs/monitor-core.c
> > +++ 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;
> > +}
> 
> Is this meant to be called?  If not, abort().

error_report() and friends are supposed to be called pretty much
everywhere, so I'd say yes.

Kevin