Re: [PATCH 5/6] trace-state: [simple] add "-trace events" argument to control initial state

On Mon, Apr 4, 2011 at 10:49 PM, Lluís <xscript@gmx.net> wrote:
> When using the "simple" tracing backend, all events are in disabled state by
> default.
>
> The "-trace events" argument can be used to provide a file with a list of trace
> event names that will be enabled prior to starting execution. This saves the
> user from manually toggling event states through the monitor interface, as well
> as enables early tracing for the selected points, much like other
> more-sophisticated backends like "ust" or "dtrace".
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---
>  docs/tracing.txt |    5 +++
>  qemu-config.c    |    5 ++-
>  qemu-options.hx  |   18 ++++++++--
>  vl.c             |   94 +++++++++++++++++++++++++++++++++++-------------------
>  4 files changed, 84 insertions(+), 38 deletions(-)

Too many whitespace/indentation changes here.  If you want to fix
whitespace then please do it in a separate patch that is not part of
this series.

> @@ -2833,9 +2834,10 @@ int main(int argc, char **argv, char **envp)
>                 break;
>  #ifdef CONFIG_SIMPLE_TRACE
>             case QEMU_OPTION_trace:
> -                opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 0);
> +                opts = qemu_opts_parse(qemu_find_opts("trace"), optarg, 1);

Previously "qemu -trace asdf" failed:
qemu: -trace asdf: Invalid parameter 'asdf'

Please don't add an implied file= since there are no legacy users whom
this would help.  Let's strictly accept file= and events=.

> @@ -2887,6 +2889,32 @@ int main(int argc, char **argv, char **envp)
>     if (!st_init(trace_file)) {
>         fprintf(stderr, "warning: unable to initialize simple trace backend\n");
>     }
> +    if (trace_events_file) {
> +        FILE *trace_events_fp = fopen(trace_events_file, "r");
> +        if (!trace_events_fp) {
> +            fprintf(stderr, "could not open trace events file '%s': %s\n",
> +                    trace_events_file, strerror(errno));
> +            exit(1);
> +        }
> +        char line_buf[1024];
> +        char *line;
> +        for (line = fgets(line_buf, 1024, trace_events_fp); line != NULL;
> +             line = fgets(line_buf, 1024, trace_events_fp)) {

Without the line variable, hardcoded 1024, and duplicated fgets() call:
while (fgets(line_buf, sizeof(line_buf), trace_events_fp)) {

> +            int len = strlen(line);

There's no reason to cast the size_t return value to int here.

> +            if (len > 1) {              /* skip empty lines */
> +                line[len - 1] = '\0';
> +                if (!st_change_trace_event_state(line, true)) {

The build breaks when --enable-trace-backend != simple because this
code is outside an #ifdef CONFIG_SIMPLE_TRACE.  Please add this:

Stefan Hajnoczi writes:

>> +            if (len > 1) {              /* skip empty lines */
>> +                line[len - 1] = '\0';
>> +                if (!st_change_trace_event_state(line, true)) {

> The build breaks when --enable-trace-backend != simple because this
> code is outside an #ifdef CONFIG_SIMPLE_TRACE.  Please add this:

> diff --git a/simpletrace.h b/simpletrace.h
> index 8d893bd..5d9d2ec 100644
> --- a/simpletrace.h
> +++ b/simpletrace.h
> @@ -43,6 +43,11 @@ static inline bool st_init(const char *file)
>  {
>      return true;
>  }
> +
> +static bool st_change_trace_event_state(const char *tname, bool tstate)
> +{
> +    return true;
> +}
>  #endif /* !CONFIG_SIMPLE_TRACE */

>  #endif /* SIMPLETRACE_H */

Hmmm... why don't simply conditionally call st_init (put it into an
#ifdef) and remove the "#else" in simpletrace.h.

I've looked at it and it's not called from anywhere else.

This also reminds me that I didn't see any "-trace" option parsing in
the OS-specific frontends (at least in linux-user).


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

On Wed, Apr 6, 2011 at 3:15 PM, Lluís <xscript@gmx.net> wrote:
> Stefan Hajnoczi writes:
>
>>> +            if (len > 1) {              /* skip empty lines */
>>> +                line[len - 1] = '\0';
>>> +                if (!st_change_trace_event_state(line, true)) {
>
>> The build breaks when --enable-trace-backend != simple because this
>> code is outside an #ifdef CONFIG_SIMPLE_TRACE.  Please add this:
>
>> diff --git a/simpletrace.h b/simpletrace.h
>> index 8d893bd..5d9d2ec 100644
>> --- a/simpletrace.h
>> +++ b/simpletrace.h
>> @@ -43,6 +43,11 @@ static inline bool st_init(const char *file)
>>  {
>>      return true;
>>  }
>> +
>> +static bool st_change_trace_event_state(const char *tname, bool tstate)
>> +{
>> +    return true;
>> +}
>>  #endif /* !CONFIG_SIMPLE_TRACE */
>
>>  #endif /* SIMPLETRACE_H */
>
> Hmmm... why don't simply conditionally call st_init (put it into an
> #ifdef) and remove the "#else" in simpletrace.h.
>
> I've looked at it and it's not called from anywhere else.

The benefit to stubbing out these functions is that callers don't have
#ifdefs.  And caller code is always built (i.e. syntax checked by the
parser) so it helps avoid bitrot.  vl.c:main() is already a long and
ugly function so it would be nice to avoid #ifdefs there.

> This also reminds me that I didn't see any "-trace" option parsing in
> the OS-specific frontends (at least in linux-user).

User emulation does not have any way to control simpletrace today.

Stefan

Stefan Hajnoczi writes:

>> Hmmm... why don't simply conditionally call st_init (put it into an
>> #ifdef) and remove the "#else" in simpletrace.h.
>>
>> I've looked at it and it's not called from anywhere else.

> The benefit to stubbing out these functions is that callers don't have
> #ifdefs.  And caller code is always built (i.e. syntax checked by the
> parser) so it helps avoid bitrot.  vl.c:main() is already a long and
> ugly function so it would be nice to avoid #ifdefs there.

Aaah... well, I simply moved everything into ifdefs in v2, as the stderr
additions also made use of the same function names but deleted the
'#include "simpletrace.h"' in vl.c (as it's already in trace.h when such
backend is enabled).

Do you want it back?


>> This also reminds me that I didn't see any "-trace" option parsing in
>> the OS-specific frontends (at least in linux-user).

> User emulation does not have any way to control simpletrace today.

Well I was referring more to the lack of a common option parsing loop on
all frontends, as I expected all of them to use hxtool.


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