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

[Lluís Vilanova]

Stefan Hajnoczi writes:

> 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.

The description is a leftover from a previous version.


>> 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);
> }

Thanks.


>> +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.

Ok.


>> +    }
>> +
>> +    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.

Ok.


>> +            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.

Copy/paste is the root of many evils...


Thanks,
  Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth