Message ID | 20120508143837.18271.22124.stgit@fimbulvetr.bsc.es |
---|---|
State | New |
Headers | show |
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
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); + } } }
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- monitor.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)