Kevin Wolf <kw...@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.
