Re: [Qemu-devel] [PATCH for-2.12 2/8] qmp: cleanup qmp queues properly

[Eric Blake <eblake@redhat.com>]

On 03/26/2018 01:38 AM, Peter Xu wrote:
> Marc-André Lureau reported that we can have this happen:
> 
> 1. client1 connects, send command C1
> 2. client1 disconnects before getting response for C1
> 3. client2 connects, who might receive response of C1
> 
> However client2 should not receive remaining responses for client1.
> 
> Basically, we should clean up the request/response queue elements when:
> 
> - before a session established

Why here? [1]

> - after a session is closed
> - before destroying the queues
> 
> Some helpers are introduced to achieve that.  We need to make sure we're
> with the lock when operating on those queues.
> 

It would also be helpful to mention that the patch includes code motion 
to declare struct QMPRequest earlier in the file.

> Reported-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   monitor.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 59 insertions(+), 20 deletions(-)
> 

> +static void qmp_request_free(QMPRequest *req)
> +{
> +    qobject_decref(req->id);
> +    qobject_decref(req->req);
> +    g_free(req);
> +}
> +
> +static void qmp_response_free(QObject *obj)
> +{
> +    qobject_decref(obj);
> +}

Why do we need this function?  Unless you plan to add to it in later 
patches, I'd rather just inline things and directly call 
qobject_decref() at the callsites...

> +
> +/* Must with the mon->qmp.qmp_queue_lock held */
> +static void monitor_qmp_cleanup_req_queue_locked(Monitor *mon)
> +{
> +    while (!g_queue_is_empty(mon->qmp.qmp_requests)) {
> +        qmp_request_free(g_queue_pop_head(mon->qmp.qmp_requests));
> +    }
> +}
> +
> +/* Must with the mon->qmp.qmp_queue_lock held */
> +static void monitor_qmp_cleanup_resp_queue_locked(Monitor *mon)
> +{
> +    while (!g_queue_is_empty(mon->qmp.qmp_responses)) {
> +        qmp_response_free(g_queue_pop_head(mon->qmp.qmp_responses));

...here,

> +    }
> +}
> +
> +static void monitor_qmp_cleanup_queues(Monitor *mon)
> +{
> +    qemu_mutex_lock(&mon->qmp.qmp_queue_lock);
> +    monitor_qmp_cleanup_req_queue_locked(mon);
> +    monitor_qmp_cleanup_resp_queue_locked(mon);
> +    qemu_mutex_unlock(&mon->qmp.qmp_queue_lock);
> +}
> +
> +
>   static void monitor_flush_locked(Monitor *mon);
>   
>   static gboolean monitor_unblocked(GIOChannel *chan, GIOCondition cond,
> @@ -497,7 +550,7 @@ static void monitor_qmp_bh_responder(void *opaque)
>               break;
>           }
>           monitor_json_emitter_raw(response.mon, response.data);
> -        qobject_decref(response.data);
> +        qmp_response_free(response.data);

and here.

> @@ -4327,6 +4364,7 @@ static void monitor_qmp_event(void *opaque, int event)
>   
>       switch (event) {
>       case CHR_EVENT_OPENED:
> +        monitor_qmp_cleanup_queues(mon);

[1] How would something be queued to need cleanup at this point, if we 
already start with a clean queue before the first monitor, and if all 
monitor close actions clean the queue?

>           mon->qmp.commands = &qmp_cap_negotiation_commands;
>           monitor_qmp_caps_reset(mon);
>           data = get_qmp_greeting(mon);
> @@ -4335,6 +4373,7 @@ static void monitor_qmp_event(void *opaque, int event)
>           mon_refcount++;
>           break;
>       case CHR_EVENT_CLOSED:
> +        monitor_qmp_cleanup_queues(mon);
>           json_message_parser_destroy(&mon->qmp.parser);
>           json_message_parser_init(&mon->qmp.parser, handle_qmp_command);
>           mon_refcount--;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org