Re: [Qemu-devel] [PATCH v4 4/7] trace: [monitor] Use new event control interface

[Stefan Hajnoczi]

On Mon, Jun 11, 2012 at 6:20 PM, Lluís Vilanova <address@hidden> wrote:
> Lluís Vilanova writes:
>
>> Stefan Hajnoczi writes:
>>> On Tue, May 8, 2012 at 3:38 PM, Lluís Vilanova <address@hidden> wrote:
>>>> Signed-off-by: Lluís Vilanova <address@hidden>
>>>> ---
>>>>  monitor.c |   15 ++++++++++++---
>>>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 8946a10..86c2538 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -625,10 +625,19 @@ 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");
>>>> -    int ret = trace_event_set_state(tp_name, new_state);
>>>>
>>>> -    if (!ret) {
>>>> -        monitor_printf(mon, "unknown event name \"%s\"\n", tp_name);
>>>> +    if (trace_event_is_pattern(tp_name)) {
>>>> +        TraceEvent *ev = NULL;
>>>> +        while ((ev = trace_event_pattern(tp_name, ev)) != NULL) {
>>>> +            trace_event_set_state_dynamic(ev, new_state);
>>>> +        }
>>>> +    } else {
>>>> +        TraceEvent *ev = trace_event_name(tp_name);
>>>> +        if (ev == NULL) {
>>>> +            monitor_printf(mon, "unknown event name \"%s\"\n", tp_name);
>>>> +        } else {
>>>> +            trace_event_set_state_dynamic(ev, new_state);
>>>> +        }
>
>>> Why check for a pattern and split the code in two?  How about just:
>
>>> while ((ev = trace_event_pattern(tp_name, ev)) != NULL) {
>>> ...
>>> }
>
>>> That should cover both the single trace event name case and the wildcard 
>>> case.
>
>> That's true... it's just that somehow I thought it was abusive to use pattern
>> matching on a string without patterns :)
>
> When going through the code to add your comments I realized why it made sense 
> to
> use 'trace_event_name' instead of 'trace_event_pattern'.
>
> In the case the string contains no pattern, not finding a result is an error 
> in
> 'trace_backend_init_events' (when reading the list of events given in the
> commandline file), as well as in 'do_trace_event_set_state' (when setting the
> state from the monitor, although here the error is not fatal).
>
> In comparison, setting by pattern never fails.

I see.  Then the single code-path implementation looks like this:

bool found = false;
while ((ev = trace_event_pattern(tp_name, ev)) != NULL) {
   found = true;
   ...
}
if (!found && !trace_event_is_pattern(tp_name)) {
   fprintf(stderr, "ERROR!");
}

It's up to you which you prefer.

Stefan