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

[Kevin Wolf]

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.

Kevin