Re: [PATCH 17/21] monitor: capture monitor commands and events in data recorder

["Dr. David Alan Gilbert" <dave@treblig.org>]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> QMP monitor commands which change system state (ie not query-XXX
> commands) are relevant to the data recorder, as they reflect what
> has been done to QEMU at runtime by the mgmt application.

Do you have any filters for avoiding capturing passwords/keys
(which admitedly they should be doing by files, but hmm).

Dave

> QMP monitor events are relevant to the data recorder, as they
> reflect a combination of what has been done to QEMU at runtime by
> the mgmt application, as well as changes triggered by the guest OS.
> 
> QMP error responses are relevant either to reveal mistakes in mgmt
> apps, or to reveal problems in QEMU. If the associated QMP command
> was not already recorded, it will be recorded for any error
> response to provide context to the failure.
> 
> QMP normal responses are skipped since the data can be volumous
> and thus result in poor signal/noise ratio.
> 
> The QMP commands are never captured to a string buffer as input
> is streamed into the JSON parser and the result is spat out as
> a QObject. Thus, the commands need to be formatted back into JSON
> string format for the data recorder. This is a mild but acceptable
> performance overhead, in the overall context of handling a QMP
> command.
> 
> The QMP events are captured immediately before they are written
> back on the wire where already in string format.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  monitor/qmp.c | 67 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 8 deletions(-)
> 
> diff --git a/monitor/qmp.c b/monitor/qmp.c
> index ccb39a48d4c..659ad23d832 100644
> --- a/monitor/qmp.c
> +++ b/monitor/qmp.c
> @@ -34,6 +34,7 @@
>  #include "qobject/qjson.h"
>  #include "qobject/qlist.h"
>  #include "qom/object_interfaces.h"
> +#include "qemu/tldr.h"
>  #include "trace.h"
>  
>  /*
> @@ -80,7 +81,8 @@ static MonitorQMP *qmp_dispatcher_current_mon;
>  OBJECT_DEFINE_TYPE(MonitorQMP, monitor_qmp, MONITOR_QMP, MONITOR);
>  
>  static void monitor_qmp_cleanup_req_queue_locked(MonitorQMP *mon);
> -static void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
> +static void qmp_send_response(MonitorQMP *mon, const QDict *rsp,
> +                              const QObject *ctx);
>  
>  static void monitor_qmp_finalize(Object *obj)
>  {
> @@ -169,7 +171,7 @@ static void monitor_qmp_emit_event(Monitor *mon, QAPIEvent event, QDict *qdict)
>          }
>      }
>  
> -    qmp_send_response(qmp, qdict);
> +    qmp_send_response(qmp, qdict, NULL);
>  }
>  
>  static bool monitor_qmp_requires_iothread(const Monitor *mon)
> @@ -243,7 +245,8 @@ static void monitor_qmp_cleanup_queue_and_resume(MonitorQMP *mon)
>  
>  }
>  
> -static void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
> +static void qmp_send_response(MonitorQMP *mon, const QDict *rsp,
> +                              const QObject *ctx)
>  {
>      const QObject *data = QOBJECT(rsp);
>      GString *json;
> @@ -252,6 +255,30 @@ static void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
>      assert(json != NULL);
>      trace_monitor_qmp_respond(mon, json->str);
>  
> +    /*
> +     * Errors are always useful to record. We recorded the
> +     * command in the caller in many cases, but if not, then
> +     * 'ctx' will be non-NULL and we should record it now
> +     * for context.
> +     */
> +    if (qtldr_msg_enabled(QTLDR_MSG_BUCKET_TYPE_COMMAND) &&
> +        qdict_haskey(rsp, "error")) {
> +        if (ctx) {
> +            g_autoptr(GString) ctxstr = qobject_to_json(ctx);
> +            qtldr_msg_record(QTLDR_MSG_BUCKET_TYPE_COMMAND, ctxstr->str);
> +        }
> +        qtldr_msg_record(QTLDR_MSG_BUCKET_TYPE_COMMAND, json->str);
> +    }
> +
> +    /*
> +     * Capture events, since they represent changes
> +     * in state or behaviour
> +     */
> +    if (qtldr_msg_enabled(QTLDR_MSG_BUCKET_TYPE_EVENT) &&
> +        qdict_haskey(rsp, "event")) {
> +        qtldr_msg_record(QTLDR_MSG_BUCKET_TYPE_EVENT, json->str);
> +    }
> +
>      g_string_append_c(json, '\n');
>      monitor_puts(&mon->parent_obj, json->str);
>  
> @@ -263,10 +290,10 @@ static void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
>   * Null @rsp can only happen for commands with QCO_NO_SUCCESS_RESP.
>   * Nothing is emitted then.
>   */
> -static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
> +static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp, QObject *ctx)
>  {
>      if (rsp) {
> -        qmp_send_response(mon, rsp);
> +        qmp_send_response(mon, rsp, ctx);
>      }
>  }
>  
> @@ -276,9 +303,33 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict *rsp)
>   */
>  static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>  {
> +    QObject *ctx = req;
>      QDict *rsp;
>      QDict *error;
>  
> +    /*
> +     * Capture commands which are liable to change state,
> +     * i.e. exclude purely information query comamnds
> +     */
> +    if (qtldr_msg_enabled(QTLDR_MSG_BUCKET_TYPE_COMMAND)) {
> +        QDict *cmd = qobject_to(QDict, req);
> +        if (cmd != NULL) {
> +            const char *cmdname = qdict_get_try_str(cmd, "execute")
> +                ?: qdict_get_try_str(cmd, "exec-oob");
> +            if (cmdname != NULL &&
> +                !g_str_has_prefix(cmdname, "query-") &&
> +                !g_str_has_prefix(cmdname, "qom-list")) {
> +                g_autoptr(GString) cmdstr = qobject_to_json(req);
> +                qtldr_msg_record(QTLDR_MSG_BUCKET_TYPE_COMMAND, cmdstr->str);
> +                /*
> +                 * Prevent recording cmd twice if the later response
> +                 * is an error
> +                 */
> +                ctx = NULL;
> +            }
> +        }
> +    }
> +
>      rsp = qmp_dispatch(mon->commands, req, qmp_oob_enabled(mon),
>                         &mon->parent_obj);
>  
> @@ -294,7 +345,7 @@ static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
>          }
>      }
>  
> -    monitor_qmp_respond(mon, rsp);
> +    monitor_qmp_respond(mon, rsp, ctx);
>      qobject_unref(rsp);
>  }
>  
> @@ -455,7 +506,7 @@ void coroutine_fn monitor_qmp_dispatcher_co(void *data)
>              trace_monitor_qmp_err_in_band(error_get_pretty(req_obj->err));
>              rsp = qmp_error_response(req_obj->err);
>              req_obj->err = NULL;
> -            monitor_qmp_respond(mon, rsp);
> +            monitor_qmp_respond(mon, rsp, NULL);
>              qobject_unref(rsp);
>          }
>  
> @@ -623,7 +674,7 @@ static void monitor_qmp_event(void *opaque, QEMUChrEvent event)
>              monitor_qmp_caps_reset(mon);
>          }
>          data = qmp_greeting(mon);
> -        qmp_send_response(mon, data);
> +        qmp_send_response(mon, data, NULL);
>          qobject_unref(data);
>          break;
>      case CHR_EVENT_CLOSED:
> -- 
> 2.56.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/