[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state |
Date: |
Fri, 22 Aug 2014 14:18:02 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Thu, Aug 21, 2014 at 07:52:37PM +0200, Lluís Vilanova wrote:
> Also removes old "trace-event", "trace-file" and "info trace-events" HMP
> commands.
Is this commit description correct? I think we don't want to remove
HMP commands. It is "legacy" but users may still rely on HMP. It's
certainly used for ad-hoc debugging.
> diff --git a/monitor.c b/monitor.c
> index cdbaa60..0f605f5 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -887,19 +887,8 @@ static void do_trace_event_set_state(Monitor *mon, const
> QDict *qdict)
> const char *tp_name = qdict_get_str(qdict, "name");
> bool new_state = qdict_get_bool(qdict, "option");
>
> - bool found = false;
> - TraceEvent *ev = NULL;
> - while ((ev = trace_event_pattern(tp_name, ev)) != NULL) {
> - found = true;
> - if (!trace_event_get_state_static(ev)) {
> - monitor_printf(mon, "event \"%s\" is not traceable\n", tp_name);
> - } else {
> - trace_event_set_state_dynamic(ev, new_state);
> - }
> - }
> - if (!trace_event_is_pattern(tp_name) && !found) {
> - monitor_printf(mon, "unknown event name \"%s\"\n", tp_name);
> - }
> + /* TODO: should propagate QMP errors to HMP */
> + qmp_trace_event_set_state(tp_name, new_state, true, true, NULL);
The TODO can be resolved with:
if (errp) {
monitor_printf(mon, "%s", error_get_pretty(errp));
error_free(errp);
}
> +TraceEventStateList *qmp_trace_event_get_state(const char *name, Error
> **errp)
> +{
> + TraceEventStateList dummy = {};
> + TraceEventStateList *prev = &dummy;
> +
> + bool found = false;
> + TraceEvent *ev = NULL;
> + while ((ev = trace_event_pattern(name, ev)) != NULL) {
> + found = true;
> + TraceEventStateList *elem = g_malloc0(sizeof(*elem));
> + elem->value = g_malloc0(sizeof(*elem->value));
> + elem->value->name = g_strdup(trace_event_get_name(ev));
> + elem->value->sstatic = trace_event_get_state_static(ev);
> + elem->value->sdynamic = trace_event_get_state_dynamic(ev);
> + prev->next = elem;
> + prev = elem;
> + }
> + if (!found && !trace_event_is_pattern(name)) {
> + error_setg(errp, "unknown event \"%s\"\n", name);
\n is not needed in error_setg() message. Please remove it.
There are more instances below.
> + }
> +
> + return dummy.next;
> +}
> +
> +void qmp_trace_event_set_state(const char *name, bool state, bool
> has_keepgoing,
> + bool keepgoing, Error **errp)
> +{
> + bool error = false;
> + bool found = false;
> + TraceEvent *ev = NULL;
> +
> + /* Check all selected events are dynamic */
> + while ((ev = trace_event_pattern(name, ev)) != NULL) {
> + found = true;
> + if (!trace_event_get_state_static(ev)) {
> + error_setg(errp, "cannot set dynamic tracing state for \"%s\"\n",
> + trace_event_get_name(ev));
error_setg() can only be called once. Calling it with non-NULL errp
produces an assertion failure.
Maybe this approach can be used instead:
while ((ev = trace_event_pattern(name, ev)) != NULL) {
found = true;
if (!(has_keepgoing && keepgoing) &&
!trace_event_get_state_static(ev)) {
error_setg(errp, "cannot set dynamic tracing state for \"%s\"",
trace_event_get_name(ev));
return;
}
}
The bool error variable can be dropped.
> + if (!(has_keepgoing && keepgoing)) {
> + error = true;
> + }
> + break;
> + }
> + }
> + if (error) {
> + return;
> + }
> + if (!found && !trace_event_is_pattern(name)) {
> + error_setg(errp, "unknown event \"%s\"\n", name);
> + return;
> + }
> +
> + if (error) {
> + return;
> + }
This condition has already been checked above. This if statement can be
dropped.
pgpDyazx0dX_m.pgp
Description: PGP signature