* Re: [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command()
2020-11-13 11:13 [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command() lichun
@ 2020-11-13 9:18 ` Kevin Wolf
2020-11-13 11:03 ` lichun
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2020-11-13 9:18 UTC (permalink / raw)
To: lichun; +Cc: qemu-devel, dgilbert
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command()
2020-11-13 9:18 ` Kevin Wolf
@ 2020-11-13 11:03 ` lichun
0 siblings, 0 replies; 3+ messages in thread
From: lichun @ 2020-11-13 11:03 UTC (permalink / raw)
To: kwolf; +Cc: qemu-devel, dgilbert
>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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command()
@ 2020-11-13 11:13 lichun
2020-11-13 9:18 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: lichun @ 2020-11-13 11:13 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, lichun, dgilbert
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;
}
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);
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);
}
static void cmd_completion(MonitorHMP *mon, const char *name, const char *list)
{
const char *p, *pstart;
char cmd[128];
int len;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-13 11:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-13 11:13 [PATCH] hmp: Update current monitor acts on the entire handle_hmp_command() lichun
2020-11-13 9:18 ` Kevin Wolf
2020-11-13 11:03 ` lichun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).