Re: [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command()

[lichun]

>Am 13.11.2020 um 12:13 hat lichun geschrieben:
>> monitor_parse_arguments() also need to known the current monitoar:
>>  (gdb) bt
>>  #0  0x0000555555ac6a6d in mon_get_cpu_sync (mon=0x0, 
>>synchronize=synchronize@entry=true) at ../monitor/misc.c:270
>>  #1  0x0000555555ac6b4a in mon_get_cpu () at ../monitor/misc.c:294
>>  #2  0x0000555555ac80fd in get_monitor_def (pval=pval@entry=0x7fffffffcc78, 
>>name=name@entry=0x7fffffffcc80 "pc") at ../monitor/misc.c:1669
>>  #3  0x000055555583fa8a in expr_unary (mon=mon@entry=0x5555568a75a0) at 
>>../monitor/hmp.c:387
>>  #4  0x000055555583fb32 in expr_prod (mon=mon@entry=0x5555568a75a0) at 
>>../monitor/hmp.c:421
>>  #5  0x000055555583fbcc in expr_logic (mon=mon@entry=0x5555568a75a0) at 
>>../monitor/hmp.c:455
>>  #6  0x000055555583f82c in expr_sum (mon=mon@entry=0x5555568a75a0) at 
>>../monitor/hmp.c:484
>>  #7  0x000055555583fc97 in get_expr (mon=mon@entry=0x5555568a75a0, 
>>pval=pval@entry=0x7fffffffce18, pp=pp@entry=0x7fffffffce08) at 
>>../monitor/hmp.c:511
>>  #8  0x00005555558409b1 in monitor_parse_arguments 
>>(mon=mon@entry=0x5555568a75a0, cmd=0x555556561e40 <hmp_cmds+7040>, 
>>cmd=0x555556561e40 <hmp_cmds+7040>, endp=0x7fffffffd288) at 
>>../monitor/hmp.c:876 
>>  #9  0x0000555555841796 in handle_hmp_command (mon=mon@entry=0x5555568a75a0, 
>>cmdline=0x5555568b12b3 "$pc", cmdline@entry=0x5555568b12b0 "xp $pc") at 
>>../monitor/hmp.c:1073
>> Therefore update current monitor as soon as possible to avoid
>> hmp/xp command failure.
>>
>> Fixes: ff04108a0e36 ("hmp: Update current monitor only in 
>> handle_hmp_command()")
>> Signed-off-by: lichun <lichun@ruijie.com.cn>
>> ---
>>  monitor/hmp.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/monitor/hmp.c b/monitor/hmp.c
>> index c5cd9d3..ee5413e 100644
>> --- a/monitor/hmp.c
>> +++ b/monitor/hmp.c
>> @@ -1072,52 +1072,52 @@ static void handle_hmp_command_co(void *opaque)
>>  }
>>
>>  void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
>>  {
>>      QDict *qdict;
>>      const HMPCommand *cmd;
>>      const char *cmd_start = cmdline;
>>
>>      trace_handle_hmp_command(mon, cmdline);
>>
>> +    /* old_mon is non-NULL when called from qmp_human_monitor_command() */
>> +    Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), &mon->common);
>> +
>>      cmd = monitor_parse_command(mon, cmdline, &cmdline, hmp_cmds);
>>      if (!cmd) {
>>          return;
>>      }
>
>Now the monitor isn't changed back in all early return cases.
>
>>
>>      qdict = monitor_parse_arguments(&mon->common, &cmdline, cmd);
>>      if (!qdict) {
>>          while (cmdline > cmd_start && qemu_isspace(cmdline[-1])) {
>>              cmdline--;
>>          }
>>          monitor_printf(&mon->common, "Try \"help %.*s\" for more 
>>information\n",
>>                         (int)(cmdline - cmd_start), cmd_start);
>>          return;
>>      }
>>
>>      if (!cmd->coroutine) {
>> -        /* old_mon is non-NULL when called from qmp_human_monitor_command() 
>> */
>> -        Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(), 
>> &mon->common);
>>          cmd->cmd(&mon->common, qdict);
>> -        monitor_set_cur(qemu_coroutine_self(), old_mon);
>>      } else {
>>          HandleHmpCommandCo data = {
>>              .mon = &mon->common,
>>              .cmd = cmd,
>>              .qdict = qdict,
>>              .done = false,
>>          };
>>          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
>> -        monitor_set_cur(co, &mon->common);
>
>Removing this line is wrong, we still need to set the current monitor
>for co, which is not qemu_coroutine_self() self.
>
>>          aio_co_enter(qemu_get_aio_context(), co);
>>          AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
>>      }
>> +    monitor_set_cur(qemu_coroutine_self(), old_mon);
>>
>>      qobject_unref(qdict);
>>  }
>
>With the above bugs fixed, this approach is one option to fix the bug.
>
>Personally, if it's possible with reasonable effort, I would prefer the
>other way, which is making sure that monitor_cur() isn't used, but the
>Monitor pointer is just passed down.  This would be a bigger change, but
>it wouldn't only fix the bug, but also clean up the code and make it
>more maintainable. 
>
>I can try to write a patch series to do it this way and see how it goes. 
This is the best way,  I will not post v2. This bug will be fixed by that 
series.
Transfer the work to you Kevin.
>
>Kevin
>