Re: [Qemu-devel] [PATCH v3] trace: [qmp] Add QAPI/QMP commands to query and control event tracing state

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4014 bytes --]

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.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]