Re: [PATCH] monitor/qmp: resume monitor when clearing its queue

[Markus Armbruster <armbru@redhat.com>]

Cc: Marc-André for additional monitor and chardev expertise.

Wolfgang Bumiller <w.bumiller@proxmox.com> writes:

> When a monitor's queue is filled up in handle_qmp_command()
> it gets suspended. It's the dispatcher bh's job currently to
> resume the monitor, which it does after processing an event
> from the queue. However, it is possible for a
> CHR_EVENT_CLOSED event to be processed before before the bh
> is scheduled, which will clear the queue without resuming
> the monitor, thereby preventing the dispatcher from reaching
> the resume() call.

Because with the request queue cleared, there's nothing for
monitor_qmp_requests_pop_any_with_lock() to pop, so
monitor_qmp_bh_dispatcher() won't look at this monitor.  It stays
suspended forever.  Correct?

Observable effect for the monitor's user?

> Fix this by resuming the monitor when clearing a queue which
> was filled up.
>
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> @Michael, we ran into this with qemu 4.0, so if the logic in this patch
> is correct it may make sense to include it in the 4.0.1 roundup.
> A backport is at [1] as 4.0 was before the monitor/ dir split.
>
> [1] https://gitlab.com/wbumiller/qemu/commit/9d8bbb5294ed084f282174b0c91e1a614e0a0714
>
>  monitor/qmp.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index 9d9e5d8b27..c1db5bf940 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -70,9 +70,19 @@ static void qmp_request_free(QMPRequest *req)
>  /* Caller must hold mon->qmp.qmp_queue_lock */
>  static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon)
>  {
> +    bool need_resume = (!qmp_oob_enabled(mon) && mon->qmp_requests->length > 0)
> +        || mon->qmp_requests->length == QMP_REQ_QUEUE_LEN_MAX;

Can you explain why this condition is correct?

>      while (!g_queue_is_empty(mon->qmp_requests)) {
>          qmp_request_free(g_queue_pop_head(mon->qmp_requests));
>      }
> +    if (need_resume) {
> +        /*
> +         * Pairs with the monitor_suspend() in handle_qmp_command() in case the
> +         * queue gets cleared from a CH_EVENT_CLOSED event before the dispatch
> +         * bh got scheduled.
> +         */
> +        monitor_resume(&mon->common);
> +    }
>  }
>  
>  static void monitor_qmp_cleanup_queues(MonitorQMP *mon)

Is monitor_qmp_cleanup_req_queue_locked() the correct place?

It's called from

* monitor_qmp_event() case CHR_EVENT_CLOSED via
  monitor_qmp_cleanup_queues(), as part of destroying the monitor's
  session state.

  This is the case you're trying to fix.  Correct?

  I figure monitor_resume() is safe because we haven't really destroyed
  anything, yet, we merely flushed the request queue.  Correct?

* monitor_data_destroy() via monitor_data_destroy_qmp() when destroying
  the monitor.

  Can need_resume be true in this case?  If yes, is monitor_resume()
  still safe?  We're in the middle of destroying the monitor...