Commit 71da4667db6 "monitor: separate QMP parser and dispatcher" moved
the handle_qmp_command tracepoint from handle_qmp_command() to
monitor_qmp_dispatch_one().  This delays tracing from enqueue time to
dequeue time.  Revert that.  Dequeue remains adequately visible via
tracepoint monitor_qmp_cmd_in_band.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Message-Id: <20180703085358.13941-14-armbru@redhat.com>
---
 monitor.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 3bf3e68bdc..3cc4b07788 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4184,12 +4184,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
 
     g_free(req_obj);
 
-    if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
-        QString *req_json = qobject_to_json(req);
-        trace_handle_qmp_command(mon, qstring_get_str(req_json));
-        qobject_unref(req_json);
-    }
-
     old_mon = cur_mon;
     cur_mon = mon;
 
@@ -4283,6 +4277,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
         qdict_del(qdict, "id");
     } /* else will fail qmp_dispatch() */
 
+    if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
+        QString *req_json = qobject_to_json(req);
+        trace_handle_qmp_command(mon, qstring_get_str(req_json));
+        qobject_unref(req_json);
+    }
+
     /* Check against the request in general layout */
     qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err);
     if (!qdict) {
-- 
2.17.1

On 3 July 2018 at 22:35, Markus Armbruster <armbru@redhat.com> wrote:
> Commit 71da4667db6 "monitor: separate QMP parser and dispatcher" moved
> the handle_qmp_command tracepoint from handle_qmp_command() to
> monitor_qmp_dispatch_one().  This delays tracing from enqueue time to
> dequeue time.  Revert that.  Dequeue remains adequately visible via
> tracepoint monitor_qmp_cmd_in_band.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20180703085358.13941-14-armbru@redhat.com>

Hi; it looks like this revert has reintroduced(?) a coverity
warning:

> ---
>  monitor.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 3bf3e68bdc..3cc4b07788 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4184,12 +4184,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>
>      g_free(req_obj);
>
> -    if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> -        QString *req_json = qobject_to_json(req);
> -        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> -        qobject_unref(req_json);
> -    }
> -
>      old_mon = cur_mon;
>      cur_mon = mon;
>
> @@ -4283,6 +4277,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>          qdict_del(qdict, "id");
>      } /* else will fail qmp_dispatch() */
>
> +    if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> +        QString *req_json = qobject_to_json(req);
> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
> +        qobject_unref(req_json);
> +    }

Coverity CID 1394216 points out that in this function a
little earlier we did a check on "if (!req && !err)", which
implies that we can get here with a NULL req pointer,
which will crash in qobject_to_json().

> +
>      /* Check against the request in general layout */
>      qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err);
>      if (!qdict) {
> --

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 3 July 2018 at 22:35, Markus Armbruster <armbru@redhat.com> wrote:
>> Commit 71da4667db6 "monitor: separate QMP parser and dispatcher" moved
>> the handle_qmp_command tracepoint from handle_qmp_command() to
>> monitor_qmp_dispatch_one().  This delays tracing from enqueue time to
>> dequeue time.  Revert that.  Dequeue remains adequately visible via
>> tracepoint monitor_qmp_cmd_in_band.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Message-Id: <20180703085358.13941-14-armbru@redhat.com>
>
> Hi; it looks like this revert has reintroduced(?) a coverity
> warning:
>
>> ---
>>  monitor.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 3bf3e68bdc..3cc4b07788 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4184,12 +4184,6 @@ static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
>>
>>      g_free(req_obj);
>>
>> -    if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> -        QString *req_json = qobject_to_json(req);
>> -        trace_handle_qmp_command(mon, qstring_get_str(req_json));
>> -        qobject_unref(req_json);
>> -    }
>> -
>>      old_mon = cur_mon;
>>      cur_mon = mon;
>>
>> @@ -4283,6 +4277,12 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
>>          qdict_del(qdict, "id");
>>      } /* else will fail qmp_dispatch() */
>>
>> +    if (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
>> +        QString *req_json = qobject_to_json(req);
>> +        trace_handle_qmp_command(mon, qstring_get_str(req_json));
>> +        qobject_unref(req_json);
>> +    }
>
> Coverity CID 1394216 points out that in this function a
> little earlier we did a check on "if (!req && !err)", which
> implies that we can get here with a NULL req pointer,
> which will crash in qobject_to_json().

Since fixed in
commit 8720e63e monitor: Fix tracepoint crash on JSON syntax error

>> +
>>      /* Check against the request in general layout */
>>      qdict = qmp_dispatch_check_obj(req, qmp_oob_enabled(mon), &err);
>>      if (!qdict) {
>> --

Thanks anyway!

On 19 July 2018 at 13:22, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> Coverity CID 1394216 points out that in this function a
>> little earlier we did a check on "if (!req && !err)", which
>> implies that we can get here with a NULL req pointer,
>> which will crash in qobject_to_json().
>
> Since fixed in
> commit 8720e63e monitor: Fix tracepoint crash on JSON syntax error

Aha, thanks. I've marked the Coverity issue as fix-submitted.

-- PMM