Marc-André Lureau <marcandre.lur...@redhat.com> writes:
> There is no need for per-command need_resume granularity, it should
> resume after running an non-oob command on oob-disabled monitor.
>
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> monitor.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 462cf96f7b..ec40a80d81 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -257,12 +257,6 @@ struct QMPRequest {
> */
> QObject *req;
> Error *err;
> - /*
> - * Whether we need to resume the monitor afterward. This flag is
> - * used to emulate the old QMP server behavior that the current
> - * command must be completed before execution of the next one.
> - */
> - bool need_resume;
Note for later: this comment goes away without a replacement.
> };
> typedef struct QMPRequest QMPRequest;
>
> @@ -4079,11 +4073,13 @@ static void monitor_qmp_bh_dispatcher(void *data)
> {
> QMPRequest *req_obj = monitor_qmp_requests_pop_any();
> QDict *rsp;
> + bool need_resume;
>
> if (!req_obj) {
> return;
> }
>
> + need_resume = !qmp_oob_enabled(req_obj->mon);
> if (req_obj->req) {
> trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?:
> "");
> monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
> @@ -4094,7 +4090,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> qobject_unref(rsp);
> }
>
> - if (req_obj->need_resume) {
> + if (need_resume) {
> /* Pairs with the monitor_suspend() in handle_qmp_command() */
Why not
if (!qmp_oob_enabled(req_obj->mon)) {
and drop the variable? Perhaps you keep the variable to have its name
provide a hint on why we resume. But even with that hint and the "pairs
with" comment, it's still less than clear. What about...
> monitor_resume(req_obj->mon);
> }
> @@ -4146,7 +4142,6 @@ static void handle_qmp_command(JSONMessageParser
> *parser, GQueue *tokens)
> req_obj->id = id;
> req_obj->req = req;
> req_obj->err = err;
> - req_obj->need_resume = false;
>
> /* Protect qmp_requests and fetching its length. */
> qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> @@ -4159,7 +4154,6 @@ static void handle_qmp_command(JSONMessageParser
> *parser, GQueue *tokens)
> */
> if (!qmp_oob_enabled(mon)) {
... adding a replacement for the deleted comment here, say
/*
* Emulate traditional QMP server behavior: read next command
* only after current command completed. Pairs with
* monitor_resume() in monitor_qmp_bh_dispatcher().
*/
Would you then be okay with cutting out the variable?
> monitor_suspend(mon);
> - req_obj->need_resume = true;
> } else {
> /* Drop the request if queue is full. */
> if (mon->qmp.qmp_requests->length >= QMP_REQ_QUEUE_LEN_MAX) {
Your patch works fine as far as I can tell, so
Reviewed-by: Markus Armbruster <arm...@redhat.com>
