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