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

[Markus Armbruster <armbru@redhat.com>]

Kevin Wolf <kwolf@redhat.com> writes:

> 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'.

I actually compile-tested before I sent this.  For once ;)

Here's container_of():

    #define container_of(ptr, type, member) ({                      \
            const typeof(((type *) 0)->member) *__mptr = (ptr);     \
            (type *) ((char *) __mptr - offsetof(type, member));})

Its first argument's only use is as an initializer for a pointer
variable.  Both type * and void * work fine there.

>> > 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.

Okay.