[v4,4/7] trace: [monitor] Use new event control interface

Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
---
 monitor.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

On Tue, May 8, 2012 at 3:38 PM, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  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.

Stefan

Stefan Hajnoczi writes:

> On Tue, May 8, 2012 at 3:38 PM, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---
>>  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 :)



Lluis

Lluís Vilanova writes:

> Stefan Hajnoczi writes:
>> On Tue, May 8, 2012 at 3:38 PM, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>> ---
>>>  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.


Lluis

On Mon, Jun 11, 2012 at 6:20 PM, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
> Lluís Vilanova writes:
>
>> Stefan Hajnoczi writes:
>>> On Tue, May 8, 2012 at 3:38 PM, Lluís Vilanova <vilanova@ac.upc.edu> wrote:
>>>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>>>> ---
>>>>  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