Patchwork Re: [PATCH 01/14] trace: Add trace-events file for declaring trace events

login
register
mail settings
Submitter Blue Swirl
Date Aug. 30, 2010, 9:01 p.m.
Message ID <AANLkTi=veEuNzENFGVBRDMSRBrv4Ks9w9MF62uB7K-XO@mail.gmail.com>
Download mbox | patch
Permalink /patch/63128/
State New
Headers show

Comments

Blue Swirl - Aug. 30, 2010, 9:01 p.m.
On Mon, Aug 30, 2010 at 8:42 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Aug 30, 2010 at 8:10 PM, malc <av1474@comtv.ru> wrote:
>> On Mon, 30 Aug 2010, Blue Swirl wrote:
>>
>>> On Mon, Aug 30, 2010 at 1:27 PM, Stefan Hajnoczi
>>> <stefanha@linux.vnet.ibm.com> wrote:
>>> > This patch introduces the trace-events file where trace events can be
>>> > declared like so:
>>> >
>>> > qemu_malloc(size_t size) "size %zu"
>>> > qemu_free(void *ptr) "ptr %p"
>>> >
>>> > These trace event declarations are processed by a new tool called
>>> > tracetool to generate code for the trace events.  Trace event
>>> > declarations are independent of the backend tracing system (LTTng User
>>> > Space Tracing, ftrace markers, DTrace).
>>>
>>> I think the tool does not work if 'sh' is not 'bash'. For example, on
>>> OpenBSD I got:
>>
>> Well, it does work with ash.
>>
>>>
>>> config-host.mak is out-of-date, running configure
>>>
>>> Error: invalid trace backend
>>> Please choose a supported trace backend.
>>>
>>>   GEN   trace.h
>>> /src/qemu/tracetool[176]: no closing quote
>>>
>>> This shows the problem:
>>> sh -x ../tracetool --nop --check-backend
>>> + set -f
>>> ../tracetool[176]: no closing quote
>>
>> `set -f' is a valid construct according to:
>> http://www.opengroup.org/onlinepubs/009695399/utilities/set.html
>>
>> The problem is likely elsewhere.
>
> Right, the offending lines are:
>    echo ${1%%(*}
> and
>    args=${1#*(}
>
> If I remove both of those, the errors are gone.
>

This patch fixes the problem. Double quotes do not help.
Stefan Hajnoczi - Aug. 31, 2010, 8:46 a.m.
On Mon, Aug 30, 2010 at 10:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Aug 30, 2010 at 8:42 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Mon, Aug 30, 2010 at 8:10 PM, malc <av1474@comtv.ru> wrote:
>>> On Mon, 30 Aug 2010, Blue Swirl wrote:
>>>
>>>> On Mon, Aug 30, 2010 at 1:27 PM, Stefan Hajnoczi
>>>> <stefanha@linux.vnet.ibm.com> wrote:
>>>> > This patch introduces the trace-events file where trace events can be
>>>> > declared like so:
>>>> >
>>>> > qemu_malloc(size_t size) "size %zu"
>>>> > qemu_free(void *ptr) "ptr %p"
>>>> >
>>>> > These trace event declarations are processed by a new tool called
>>>> > tracetool to generate code for the trace events.  Trace event
>>>> > declarations are independent of the backend tracing system (LTTng User
>>>> > Space Tracing, ftrace markers, DTrace).
>>>>
>>>> I think the tool does not work if 'sh' is not 'bash'. For example, on
>>>> OpenBSD I got:
>>>
>>> Well, it does work with ash.
>>>
>>>>
>>>> config-host.mak is out-of-date, running configure
>>>>
>>>> Error: invalid trace backend
>>>> Please choose a supported trace backend.
>>>>
>>>>   GEN   trace.h
>>>> /src/qemu/tracetool[176]: no closing quote
>>>>
>>>> This shows the problem:
>>>> sh -x ../tracetool --nop --check-backend
>>>> + set -f
>>>> ../tracetool[176]: no closing quote
>>>
>>> `set -f' is a valid construct according to:
>>> http://www.opengroup.org/onlinepubs/009695399/utilities/set.html
>>>
>>> The problem is likely elsewhere.
>>
>> Right, the offending lines are:
>>    echo ${1%%(*}
>> and
>>    args=${1#*(}
>>
>> If I remove both of those, the errors are gone.
>>
>
> This patch fixes the problem. Double quotes do not help.
>
> diff --git a/tracetool b/tracetool
> index d640100..01de580 100755
> --- a/tracetool
> +++ b/tracetool
> @@ -29,14 +29,14 @@ EOF
>  # Get the name of a trace event
>  get_name()
>  {
> -    echo ${1%%(*}
> +    echo ${1%%\(*}
>  }
>
>  # Get the argument list of a trace event, including types and names
>  get_args()
>  {
>     local args
> -    args=${1#*(}
> +    args=${1#*\(}
>     args=${args%)*}
>     echo "$args"
>  }

Thanks for finding and fixing this!  I have been testing with dash in
addition to bash.

I'd like to write tracetool in Python (or Perl, if needed) to
eliminate issues like this.  I chose shell though to avoid the
dependency on Python.  QEMU currently uses Perl for texi2pod.pl and
Python for QMP scripts but both are optional.  Tracetool isn't
optional because it runs even when tracing is disabled ("nop" backend
generates empty stub functions).  How do you feel about tracetool in
Python?

Stefan
Blue Swirl - Aug. 31, 2010, 6 p.m.
On Tue, Aug 31, 2010 at 8:46 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Aug 30, 2010 at 10:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Mon, Aug 30, 2010 at 8:42 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Mon, Aug 30, 2010 at 8:10 PM, malc <av1474@comtv.ru> wrote:
>>>> On Mon, 30 Aug 2010, Blue Swirl wrote:
>>>>
>>>>> On Mon, Aug 30, 2010 at 1:27 PM, Stefan Hajnoczi
>>>>> <stefanha@linux.vnet.ibm.com> wrote:
>>>>> > This patch introduces the trace-events file where trace events can be
>>>>> > declared like so:
>>>>> >
>>>>> > qemu_malloc(size_t size) "size %zu"
>>>>> > qemu_free(void *ptr) "ptr %p"
>>>>> >
>>>>> > These trace event declarations are processed by a new tool called
>>>>> > tracetool to generate code for the trace events.  Trace event
>>>>> > declarations are independent of the backend tracing system (LTTng User
>>>>> > Space Tracing, ftrace markers, DTrace).
>>>>>
>>>>> I think the tool does not work if 'sh' is not 'bash'. For example, on
>>>>> OpenBSD I got:
>>>>
>>>> Well, it does work with ash.
>>>>
>>>>>
>>>>> config-host.mak is out-of-date, running configure
>>>>>
>>>>> Error: invalid trace backend
>>>>> Please choose a supported trace backend.
>>>>>
>>>>>   GEN   trace.h
>>>>> /src/qemu/tracetool[176]: no closing quote
>>>>>
>>>>> This shows the problem:
>>>>> sh -x ../tracetool --nop --check-backend
>>>>> + set -f
>>>>> ../tracetool[176]: no closing quote
>>>>
>>>> `set -f' is a valid construct according to:
>>>> http://www.opengroup.org/onlinepubs/009695399/utilities/set.html
>>>>
>>>> The problem is likely elsewhere.
>>>
>>> Right, the offending lines are:
>>>    echo ${1%%(*}
>>> and
>>>    args=${1#*(}
>>>
>>> If I remove both of those, the errors are gone.
>>>
>>
>> This patch fixes the problem. Double quotes do not help.
>>
>> diff --git a/tracetool b/tracetool
>> index d640100..01de580 100755
>> --- a/tracetool
>> +++ b/tracetool
>> @@ -29,14 +29,14 @@ EOF
>>  # Get the name of a trace event
>>  get_name()
>>  {
>> -    echo ${1%%(*}
>> +    echo ${1%%\(*}
>>  }
>>
>>  # Get the argument list of a trace event, including types and names
>>  get_args()
>>  {
>>     local args
>> -    args=${1#*(}
>> +    args=${1#*\(}
>>     args=${args%)*}
>>     echo "$args"
>>  }
>
> Thanks for finding and fixing this!  I have been testing with dash in
> addition to bash.
>
> I'd like to write tracetool in Python (or Perl, if needed) to
> eliminate issues like this.  I chose shell though to avoid the
> dependency on Python.  QEMU currently uses Perl for texi2pod.pl and
> Python for QMP scripts but both are optional.  Tracetool isn't
> optional because it runs even when tracing is disabled ("nop" backend
> generates empty stub functions).  How do you feel about tracetool in
> Python?

I'd like to avoid additional dependencies. One way to solve this could
be to ship QEMU with default-trace.[ch] which are used for the nop
case, then tracetool implementation would not matter much. But these
files would need to be updated, in synch, whenever new trace points
are added which is fragile.

Maybe a bit of CPP magic added to trace-events could simplify (or even
eliminate) the script, something like we do with .hx files.
Stefan Hajnoczi - Sept. 1, 2010, 6:18 a.m.
On Tue, Aug 31, 2010 at 7:00 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Aug 31, 2010 at 8:46 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Mon, Aug 30, 2010 at 10:01 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Mon, Aug 30, 2010 at 8:42 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Mon, Aug 30, 2010 at 8:10 PM, malc <av1474@comtv.ru> wrote:
>>>>> On Mon, 30 Aug 2010, Blue Swirl wrote:
>>>>>
>>>>>> On Mon, Aug 30, 2010 at 1:27 PM, Stefan Hajnoczi
>>>>>> <stefanha@linux.vnet.ibm.com> wrote:
>>>>>> > This patch introduces the trace-events file where trace events can be
>>>>>> > declared like so:
>>>>>> >
>>>>>> > qemu_malloc(size_t size) "size %zu"
>>>>>> > qemu_free(void *ptr) "ptr %p"
>>>>>> >
>>>>>> > These trace event declarations are processed by a new tool called
>>>>>> > tracetool to generate code for the trace events.  Trace event
>>>>>> > declarations are independent of the backend tracing system (LTTng User
>>>>>> > Space Tracing, ftrace markers, DTrace).
>>>>>>
>>>>>> I think the tool does not work if 'sh' is not 'bash'. For example, on
>>>>>> OpenBSD I got:
>>>>>
>>>>> Well, it does work with ash.
>>>>>
>>>>>>
>>>>>> config-host.mak is out-of-date, running configure
>>>>>>
>>>>>> Error: invalid trace backend
>>>>>> Please choose a supported trace backend.
>>>>>>
>>>>>>   GEN   trace.h
>>>>>> /src/qemu/tracetool[176]: no closing quote
>>>>>>
>>>>>> This shows the problem:
>>>>>> sh -x ../tracetool --nop --check-backend
>>>>>> + set -f
>>>>>> ../tracetool[176]: no closing quote
>>>>>
>>>>> `set -f' is a valid construct according to:
>>>>> http://www.opengroup.org/onlinepubs/009695399/utilities/set.html
>>>>>
>>>>> The problem is likely elsewhere.
>>>>
>>>> Right, the offending lines are:
>>>>    echo ${1%%(*}
>>>> and
>>>>    args=${1#*(}
>>>>
>>>> If I remove both of those, the errors are gone.
>>>>
>>>
>>> This patch fixes the problem. Double quotes do not help.
>>>
>>> diff --git a/tracetool b/tracetool
>>> index d640100..01de580 100755
>>> --- a/tracetool
>>> +++ b/tracetool
>>> @@ -29,14 +29,14 @@ EOF
>>>  # Get the name of a trace event
>>>  get_name()
>>>  {
>>> -    echo ${1%%(*}
>>> +    echo ${1%%\(*}
>>>  }
>>>
>>>  # Get the argument list of a trace event, including types and names
>>>  get_args()
>>>  {
>>>     local args
>>> -    args=${1#*(}
>>> +    args=${1#*\(}
>>>     args=${args%)*}
>>>     echo "$args"
>>>  }
>>
>> Thanks for finding and fixing this!  I have been testing with dash in
>> addition to bash.
>>
>> I'd like to write tracetool in Python (or Perl, if needed) to
>> eliminate issues like this.  I chose shell though to avoid the
>> dependency on Python.  QEMU currently uses Perl for texi2pod.pl and
>> Python for QMP scripts but both are optional.  Tracetool isn't
>> optional because it runs even when tracing is disabled ("nop" backend
>> generates empty stub functions).  How do you feel about tracetool in
>> Python?
>
> I'd like to avoid additional dependencies. One way to solve this could
> be to ship QEMU with default-trace.[ch] which are used for the nop
> case, then tracetool implementation would not matter much. But these
> files would need to be updated, in synch, whenever new trace points
> are added which is fragile.
>
> Maybe a bit of CPP magic added to trace-events could simplify (or even
> eliminate) the script, something like we do with .hx files.

Okay, let's stick to tracetool in POSIX shell.  I'll resend this patch
with your fix included.

Stefan

Patch

diff --git a/tracetool b/tracetool
index d640100..01de580 100755
--- a/tracetool
+++ b/tracetool
@@ -29,14 +29,14 @@  EOF
 # Get the name of a trace event
 get_name()
 {
-    echo ${1%%(*}
+    echo ${1%%\(*}
 }

 # Get the argument list of a trace event, including types and names
 get_args()
 {
     local args
-    args=${1#*(}
+    args=${1#*\(}
     args=${args%)*}
     echo "$args"
 }