| Message ID | 541C8B7E-1E25-479E-B69E-A4B58BFA45F3@nowonline.co.uk |
|---|---|
| State | New |
| Headers | show |
Am 16.03.2012 13:29, schrieb Lee Essen: > Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> > > --- > > scripts/tracetool | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/scripts/tracetool b/scripts/tracetool > index 65bd0a1..2e43d05 100755 > --- a/scripts/tracetool > +++ b/scripts/tracetool > @@ -123,7 +123,7 @@ get_argc() > # Get the format string including double quotes for a trace event > get_fmt() > { > - puts "${1#*)}" > + puts "${1#*}" > } > > linetoh_begin_nop() > Cc'ing the trace maintainer. I assume Lee forgot to look up the maintainer, but Stefan, my checking MAINTAINERS indicates tracetool is missing in the Tracing section too. Could you add it please? Not being a shell expert I can't judge what this is actually trying to do. Note that there is also an effort underway to rewrite tracetool as tracetool.py. Andreas
On 16 Mar 2012, at 12:44, Andreas Färber wrote: > Am 16.03.2012 13:29, schrieb Lee Essen: >> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> >> >> --- >> >> scripts/tracetool | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/scripts/tracetool b/scripts/tracetool >> index 65bd0a1..2e43d05 100755 >> --- a/scripts/tracetool >> +++ b/scripts/tracetool >> @@ -123,7 +123,7 @@ get_argc() >> # Get the format string including double quotes for a trace event >> get_fmt() >> { >> - puts "${1#*)}" >> + puts "${1#*}" >> } >> >> linetoh_begin_nop() >> > Cc'ing the trace maintainer. I assume Lee forgot to look up the > maintainer, but Stefan, my checking MAINTAINERS indicates tracetool is > missing in the Tracing section too. Could you add it please? > > Not being a shell expert I can't judge what this is actually trying to > do. Note that there is also an effort underway to rewrite tracetool as > tracetool.py. > > Andreas > Actually, I think I need to slow down a bit… there are more problems than just that bracket… # make GEN trace.h /tmp/patch/qemu/scripts/tracetool[520]: local: not found [No such file or directory] /tmp/patch/qemu/scripts/tracetool[66]: local: not found [No such file or directory] /tmp/patch/qemu/scripts/tracetool[136]: local: not found [No such file or directory] /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] /tmp/patch/qemu/scripts/tracetool[80]: local: not found [No such file or directory] /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] From what I can see "local" isn't supported in posix ... "The POSIX standard supports functions, as shown above, but the semantics are weaker: functions do not have local traps or options, it is not possible to define local variables, and functions can't be exported." So I could do with some advice now on how to proceed … is the goal to keep posix shell compliance? Wait for a tracetool.py version? Or should I go back to messing with SHELL? Regards, Lee.
Il 16/03/2012 13:29, Lee Essen ha scritto: > @@ -123,7 +123,7 @@ get_argc() > # Get the format string including double quotes for a trace event > get_fmt() > { > - puts "${1#*)}" > + puts "${1#*}" > } > Eric, can you look at this? Is it a bashism or a Solaris bug? I would write it, to be entirely safe, as local fmt fmt=${1#*\)} puts "$fmt" where I'm using the extra variable to avoid the ambiguity of quoting the parentheses within quotes; variable assignments are always implicitly quoted. Paolo
Il 16/03/2012 14:00, Lee Essen ha scritto: > /tmp/patch/qemu/scripts/tracetool[520]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[66]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[136]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[80]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] > > From what I can see "local" isn't supported in posix ... > "The POSIX standard supports functions, as shown above, but the semantics are weaker: functions do not have local traps or options, it is not possible to define local variables, and functions can't be exported." > > So I could do with some advice now on how to proceed … is the goal to keep posix shell compliance? Wait for a tracetool.py version? Or should I go back to messing with SHELL? I think #!/bin/bash is a better solution in the short-term. Paolo
Am 16.03.2012 14:00, schrieb Lee Essen: > On 16 Mar 2012, at 12:44, Andreas Färber wrote: > >> Am 16.03.2012 13:29, schrieb Lee Essen: >>> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> >>> >>> --- >>> >>> scripts/tracetool | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> diff --git a/scripts/tracetool b/scripts/tracetool >>> index 65bd0a1..2e43d05 100755 >>> --- a/scripts/tracetool >>> +++ b/scripts/tracetool >>> @@ -123,7 +123,7 @@ get_argc() >>> # Get the format string including double quotes for a trace event >>> get_fmt() >>> { >>> - puts "${1#*)}" >>> + puts "${1#*}" >>> } >>> >>> linetoh_begin_nop() >>> >> Cc'ing the trace maintainer. I assume Lee forgot to look up the >> maintainer, but Stefan, my checking MAINTAINERS indicates tracetool is >> missing in the Tracing section too. Could you add it please? >> >> Not being a shell expert I can't judge what this is actually trying to >> do. Note that there is also an effort underway to rewrite tracetool as >> tracetool.py. > > Actually, I think I need to slow down a bit… :) No need to rush, it's been broken for a while. > there are more problems than just that bracket… > > # make > GEN trace.h > /tmp/patch/qemu/scripts/tracetool[520]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[66]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[136]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[80]: local: not found [No such file or directory] > /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] > > From what I can see "local" isn't supported in posix ... > "The POSIX standard supports functions, as shown above, but the semantics are weaker: functions do not have local traps or options, it is not possible to define local variables, and functions can't be exported." Hm, Blue's patch in the bug I referenced earlier still had local in it and according to my comments worked with Solaris 10's /usr/xpg4/bin/sh, and I don't have that issue on OpenIndiana (bash). What shell did you test with on SmartOS? > So I could do with some advice now on how to proceed … is the goal to keep posix shell compliance? Wait for a tracetool.py version? Or should I go back to messing with SHELL? I'd recommend to evaluate what needs to be done to make the script(s) POSIX-compliant. If the resulting patch is reasonable then IMO we should apply it even if it gets replaced by a Python version later (it was still feature-incomplete last time posted and has been floating around a while already). Was just pointing it out for you in case it's easier to get running on your system. Andreas
Am 16.03.2012 14:19, schrieb Paolo Bonzini: > Il 16/03/2012 14:00, Lee Essen ha scritto: >> /tmp/patch/qemu/scripts/tracetool[520]: local: not found [No such file or directory] >> /tmp/patch/qemu/scripts/tracetool[66]: local: not found [No such file or directory] >> /tmp/patch/qemu/scripts/tracetool[136]: local: not found [No such file or directory] >> /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] >> /tmp/patch/qemu/scripts/tracetool[80]: local: not found [No such file or directory] >> /tmp/patch/qemu/scripts/tracetool[55]: local: not found [No such file or directory] >> >> From what I can see "local" isn't supported in posix ... >> "The POSIX standard supports functions, as shown above, but the semantics are weaker: functions do not have local traps or options, it is not possible to define local variables, and functions can't be exported." >> >> So I could do with some advice now on how to proceed … is the goal to keep posix shell compliance? Wait for a tracetool.py version? Or should I go back to messing with SHELL? > > I think #!/bin/bash is a better solution in the short-term. Breaks if it's in /usr/gnu/bin or /opt/SUNWfoo/bin. :) Just like Python scripts can't rely on #!/usr/bin/env python on BeOS/Haiku in lack of /usr. The world is complicated. Andreas
On 03/16/2012 07:18 AM, Paolo Bonzini wrote: > Il 16/03/2012 13:29, Lee Essen ha scritto: >> @@ -123,7 +123,7 @@ get_argc() >> # Get the format string including double quotes for a trace event >> get_fmt() >> { >> - puts "${1#*)}" This says to call puts with the first argument of get_fmt, except with the shortest prefix ending in ) omitted. Or are you complaining that there is a shell treating this as a syntax error? >> + puts "${1#*}" This says to omit the shortest prefix that matches the glob '*', but that is always the empty string, so you might as well write it "$1". >> } >> > > Eric, can you look at this? Is it a bashism or a Solaris bug? Solaris /bin/sh lacks support for ${var#pattern}, but POSIX requires it. If this is using #!/bin/sh, you aren't portable. > > I would write it, to be entirely safe, as > > local fmt local is not portable. > fmt=${1#*\)} > puts "$fmt" This looks reasonable, if you were hitting syntax errors on an unquoted ), and if you are sure that you have a POSIX rather than Solaris /bin/sh. > > where I'm using the extra variable to avoid the ambiguity of quoting the > parentheses within quotes; variable assignments are always implicitly > quoted. > > Paolo >
On Fri, Mar 16, 2012 at 12:29 PM, Lee Essen <lee.essen@nowonline.co.uk> wrote: > Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> > > --- > > scripts/tracetool | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) I'm going to spend some time today reviewing recent tracing patches. I'd prefer to move to a Python version of tracetool rather than worry about the shell quirks across all host platforms. If the Python rewrite cannot be merged for 1.1 then it makes sense to go with shell portability fix. Stefan
On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: > On Fri, Mar 16, 2012 at 12:29 PM, Lee Essen <lee.essen@nowonline.co.uk> wrote: >> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> >> >> --- >> >> scripts/tracetool | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) > > I'm going to spend some time today reviewing recent tracing patches. > I'd prefer to move to a Python version of tracetool rather than worry > about the shell quirks across all host platforms. > > If the Python rewrite cannot be merged for 1.1 then it makes sense to > go with shell portability fix. Hi Stefan, While you are looking at this, there are a couple of other related issues worth having in the back of your mind: 1. "self" is a reserved word in Solaris/Illumos trace, and it's used in a few trace calls. 2. "bool" isn't recognised by default, again used in a couple of traces ... could be fixed by typedef, but switching to int is probably better (imho) 3. Some work is needed on the linking stage when using Solaris/Illumos dtrace. You need to provide all the objs to the dtrace -G call in order to get an object generated that includes all of the relevant symbols in it. Happy to provide more detail if needed. Cheers, Lee.
On Mon, Mar 19, 2012 at 12:05 PM, Lee Essen <lee.essen@nowonline.co.uk> wrote: > On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: > >> On Fri, Mar 16, 2012 at 12:29 PM, Lee Essen <lee.essen@nowonline.co.uk> wrote: >>> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> >>> >>> --- >>> >>> scripts/tracetool | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> I'm going to spend some time today reviewing recent tracing patches. >> I'd prefer to move to a Python version of tracetool rather than worry >> about the shell quirks across all host platforms. >> >> If the Python rewrite cannot be merged for 1.1 then it makes sense to >> go with shell portability fix. > > Hi Stefan, > > While you are looking at this, there are a couple of other related issues worth having in the back of your mind: > > 1. "self" is a reserved word in Solaris/Illumos trace, and it's used in a few trace calls. > 2. "bool" isn't recognised by default, again used in a couple of traces ... could be fixed by typedef, but switching to int is probably better (imho) > 3. Some work is needed on the linking stage when using Solaris/Illumos dtrace. You need to provide all the objs to the dtrace -G call in order to get an object generated that includes all of the relevant symbols in it. Happy to provide more detail if needed. If you already have patches for these points then please send them. You, I, or someone else can port them to Python. Stefan
Am 19.03.2012 13:05, schrieb Lee Essen: > On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: > >> I'm going to spend some time today reviewing recent tracing patches. >> I'd prefer to move to a Python version of tracetool rather than worry >> about the shell quirks across all host platforms. >> >> If the Python rewrite cannot be merged for 1.1 then it makes sense to >> go with shell portability fix. > Hi Stefan, > > While you are looking at this, there are a couple of other related issues worth having in the back of your mind: > > 1. "self" is a reserved word in Solaris/Illumos trace, and it's used in a few trace calls. > 2. "bool" isn't recognised by default, again used in a couple of traces ... could be fixed by typedef, but switching to int is probably better (imho) > 3. Some work is needed on the linking stage when using Solaris/Illumos dtrace. You need to provide all the objs to the dtrace -G call in order to get an object generated that includes all of the relevant symbols in it. Happy to provide more detail if needed. "Reviewing recent tracing patches" does not imply writing patches to fix everyone's issues, you'll likely need to send patches for those issues yourself at some point (or me if I find time, or someone else). Stopping to use bool throughout QEMU is not an option IMO. If it's limited to the DTrace backend code and doesn't negatively affect SystemTap then that may be an option. Andreas
Am 19.03.2012 13:05, schrieb Lee Essen: > On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: > >> On Fri, Mar 16, 2012 at 12:29 PM, Lee Essen >> <lee.essen@nowonline.co.uk> wrote: >>> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> >>> >>> --- >>> >>> scripts/tracetool | 2 +- >>> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> I'm going to spend some time today reviewing recent tracing patches. >> I'd prefer to move to a Python version of tracetool rather than worry >> about the shell quirks across all host platforms. >> >> If the Python rewrite cannot be merged for 1.1 then it makes sense to >> go with shell portability fix. > > Hi Stefan, > > While you are looking at this, there are a couple of other related > issues worth having in the back of your mind: > > 1. "self" is a reserved word in Solaris/Illumos trace, and it's used > in a few trace calls. > 2. "bool" isn't recognised by default, again used in a couple of > traces ... could be fixed by typedef, but switching to int is probably > better (imho) No. The correct solution is including stdbool.h which is provided by the compiler (gcc) and which defines bool and its values false and true. More boolean variables should use bool instead of int, because that improves readability of the code and saves memory in structures. Regards, Stefan W.
Am 19.03.2012 13:40, schrieb Stefan Weil: > Am 19.03.2012 13:05, schrieb Lee Essen: >> On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: >> >>> On Fri, Mar 16, 2012 at 12:29 PM, Lee Essen >>> <lee.essen@nowonline.co.uk> wrote: >>>> Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> >>>> >>>> --- >>>> >>>> scripts/tracetool | 2 +- >>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>> >>> I'm going to spend some time today reviewing recent tracing patches. >>> I'd prefer to move to a Python version of tracetool rather than worry >>> about the shell quirks across all host platforms. >>> >>> If the Python rewrite cannot be merged for 1.1 then it makes sense to >>> go with shell portability fix. >> >> Hi Stefan, >> >> While you are looking at this, there are a couple of other related >> issues worth having in the back of your mind: >> >> 1. "self" is a reserved word in Solaris/Illumos trace, and it's used >> in a few trace calls. >> 2. "bool" isn't recognised by default, again used in a couple of >> traces ... could be fixed by typedef, but switching to int is >> probably better (imho) > > No. The correct solution is including stdbool.h which is provided by the > compiler (gcc) and which defines bool and its values false and true. AFAIU the issue is not with C code but with D code: http://docs.oracle.com/cd/E19253-01/817-6223/chp-typeopexpr-2/index.html Andreas > > > More boolean variables should use bool instead of int, because that > improves > readability of the code and saves memory in structures. > > Regards, > Stefan W. >
On 19 Mar 2012, at 12:32, Andreas Färber wrote: > Am 19.03.2012 13:05, schrieb Lee Essen: >> On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: >> >>> I'm going to spend some time today reviewing recent tracing patches. >>> I'd prefer to move to a Python version of tracetool rather than worry >>> about the shell quirks across all host platforms. >>> >>> If the Python rewrite cannot be merged for 1.1 then it makes sense to >>> go with shell portability fix. >> Hi Stefan, >> >> While you are looking at this, there are a couple of other related issues worth having in the back of your mind: >> >> 1. "self" is a reserved word in Solaris/Illumos trace, and it's used in a few trace calls. >> 2. "bool" isn't recognised by default, again used in a couple of traces ... could be fixed by typedef, but switching to int is probably better (imho) >> 3. Some work is needed on the linking stage when using Solaris/Illumos dtrace. You need to provide all the objs to the dtrace -G call in order to get an object generated that includes all of the relevant symbols in it. Happy to provide more detail if needed. > "Reviewing recent tracing patches" does not imply writing patches to fix > everyone's issues, you'll likely need to send patches for those issues > yourself at some point (or me if I find time, or someone else). > > Stopping to use bool throughout QEMU is not an option IMO. If it's > limited to the DTrace backend code and doesn't negatively affect > SystemTap then that may be an option. In my original (way-too-long) patch for Illumos I included fixes for both of the above, the approach I took was to map "bool" to "int" and "self" to "_self" in the dtrace part of tracetool. So it should have had no impact on any non-dtrace stuff. If that seems like a sensible approach I will look again, and at the linking issues. Lee.
On Mon, Mar 19, 2012 at 1:35 PM, Lee Essen <lee.essen@nowonline.co.uk> wrote: > On 19 Mar 2012, at 12:32, Andreas Färber wrote: > >> Am 19.03.2012 13:05, schrieb Lee Essen: >>> On 19 Mar 2012, at 11:59, Stefan Hajnoczi wrote: >>> >>>> I'm going to spend some time today reviewing recent tracing patches. >>>> I'd prefer to move to a Python version of tracetool rather than worry >>>> about the shell quirks across all host platforms. >>>> >>>> If the Python rewrite cannot be merged for 1.1 then it makes sense to >>>> go with shell portability fix. >>> Hi Stefan, >>> >>> While you are looking at this, there are a couple of other related issues worth having in the back of your mind: >>> >>> 1. "self" is a reserved word in Solaris/Illumos trace, and it's used in a few trace calls. >>> 2. "bool" isn't recognised by default, again used in a couple of traces ... could be fixed by typedef, but switching to int is probably better (imho) >>> 3. Some work is needed on the linking stage when using Solaris/Illumos dtrace. You need to provide all the objs to the dtrace -G call in order to get an object generated that includes all of the relevant symbols in it. Happy to provide more detail if needed. >> "Reviewing recent tracing patches" does not imply writing patches to fix >> everyone's issues, you'll likely need to send patches for those issues >> yourself at some point (or me if I find time, or someone else). >> >> Stopping to use bool throughout QEMU is not an option IMO. If it's >> limited to the DTrace backend code and doesn't negatively affect >> SystemTap then that may be an option. > > > In my original (way-too-long) patch for Illumos I included fixes for both of the above, the approach I took was to map "bool" to "int" and "self" to "_self" in the dtrace part of tracetool. So it should have had no impact on any non-dtrace stuff. > > If that seems like a sensible approach I will look again, and at the linking issues. That sounds like it can work. Stefan
On 20/03/2012 16:59, Stefan Hajnoczi wrote: > On Mon, Mar 19, 2012 at 1:35 PM, Lee Essen<lee.essen@nowonline.co.uk> wrote: >> On 19 Mar 2012, at 12:32, Andreas Färber wrote: >> >>> Am 19.03.2012 13:05, schrieb Lee Essen: >>> In my original (way-too-long) patch for Illumos I included fixes for >>> both of the above, the approach I took was to map "bool" to "int" >>> and "self" to "_self" in the dtrace part of tracetool. So it should >>> have had no impact on any non-dtrace stuff. If that seems like a >>> sensible approach I will look again, and at the linking issues. > That sounds like it can work. > > Stefan There are only 5 bool's and 4 self's in the whole of the trace-events file ... is it easier to "fix" in there, or do you think a change to tracetool is the most appropriate way? Lee.
On Tue, Mar 20, 2012 at 5:11 PM, Lee Essen <lee.essen@nowonline.co.uk> wrote: > On 20/03/2012 16:59, Stefan Hajnoczi wrote: >> >> On Mon, Mar 19, 2012 at 1:35 PM, Lee Essen<lee.essen@nowonline.co.uk> >> wrote: >>> >>> On 19 Mar 2012, at 12:32, Andreas Färber wrote: >>> >>>> Am 19.03.2012 13:05, schrieb Lee Essen: >>>> In my original (way-too-long) patch for Illumos I included fixes for >>>> both of the above, the approach I took was to map "bool" to "int" and "self" >>>> to "_self" in the dtrace part of tracetool. So it should have had no impact >>>> on any non-dtrace stuff. If that seems like a sensible approach I will look >>>> again, and at the linking issues. >> >> That sounds like it can work. >> >> Stefan > > There are only 5 bool's and 4 self's in the whole of the trace-events file > ... is it easier to "fix" in there, or do you think a change to tracetool is > the most appropriate way? Folks will add more bool and maybe "self" in the future, so updating ./trace-events is not a permanent solution. tracetool has the ability to do two things: 1. It can reject invalid trace-events input. 2. It can fudge the generated trace backend code so that "self" gets turned into "self_", for example. #2 is nice when we can do it without confusing users. I suggest modifying tracetool. You may wish to hold off on this for a week because we're about to merge the Python version of tracetool, which makes it much more portable. Stefan
diff --git a/scripts/tracetool b/scripts/tracetool index 65bd0a1..2e43d05 100755 --- a/scripts/tracetool +++ b/scripts/tracetool @@ -123,7 +123,7 @@ get_argc() # Get the format string including double quotes for a trace event get_fmt() { - puts "${1#*)}" + puts "${1#*}" } linetoh_begin_nop()
Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> --- scripts/tracetool | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)