| Message ID | 1378906912-14015-1-git-send-email-gabriel@kerneis.info |
|---|---|
| State | New |
| Headers | show |
Il 11/09/2013 15:41, Gabriel Kerneis ha scritto: > The variable extra_cflags needs to be quoted in config-host.mak, > in particular because it might contain parentheses that would > otherwise be interpreted by the shell when reloading the file. > > For instance, if one wants to define some attribute with configure: > > ./configure --extra-cflags="-Dcoroutine_fn='__attribute__((coroutine_fn))'" > > A more robust approach would be to escape every variable properly, but > there is no portable equivalent to bash's "printf %q" solution. The > current patch, while not bullet-proof, works well in the common case. > > Signed-off-by: Gabriel Kerneis <gabriel@kerneis.info> Where does the shell read config-host.mak? Make does not need the quotes. Paolo
On Wed, Sep 11, 2013 at 04:01:50PM +0200, Paolo Bonzini wrote: > > ./configure --extra-cflags="-Dcoroutine_fn='__attribute__((coroutine_fn))'" > > Where does the shell read config-host.mak? Make does not need the quotes. I might have been confused about the shell vs. make interpreting the string, but I am positive that without the patch, the following fails: mkdir bin/test cd bin/test ../../configure \ --extra-cflags="-Dcoroutine_fn='__attribute__((coroutine_fn))' -w" make tests/test-coroutine touch ../../configure make tests/test-coroutine with the following error message: config-host.mak is out-of-date, running configure sh: 1: Syntax error: "(" unexpected make: *** [config-host.mak] Error 2 My patch fixes this issue.
Il 11/09/2013 16:42, Gabriel Kerneis ha scritto: > On Wed, Sep 11, 2013 at 04:01:50PM +0200, Paolo Bonzini wrote: >>> ./configure --extra-cflags="-Dcoroutine_fn='__attribute__((coroutine_fn))'" >> >> Where does the shell read config-host.mak? Make does not need the quotes. > > I might have been confused about the shell vs. make interpreting the > string, but I am positive that without the patch, the following fails: > > mkdir bin/test > cd bin/test > ../../configure \ > --extra-cflags="-Dcoroutine_fn='__attribute__((coroutine_fn))' -w" > make tests/test-coroutine > touch ../../configure > make tests/test-coroutine > > with the following error message: > > config-host.mak is out-of-date, running configure > sh: 1: Syntax error: "(" unexpected > make: *** [config-host.mak] Error 2 > > My patch fixes this issue. Oh, then it's this line in configure that has to be changed to do proper quoting. printf "# Configured with:" >> $config_host_mak printf " '%s'" "$0" "$@" >> $config_host_mak Something like for arg in "$0" "$@"; do quoted_arg=$(echo "$i" | sed 's/[$\\"]/\\&/g') printf ' "%s"' "$quoted_arg" done >> $config_host_mak could replace the second line. Adding Eric Blake in case he knows some extra trick. Paolo
On Wed, Sep 11, 2013 at 04:53:35PM +0200, Paolo Bonzini wrote: > Oh, then it's this line in configure that has to be changed to do proper > quoting. > > printf "# Configured with:" >> $config_host_mak > printf " '%s'" "$0" "$@" >> $config_host_mak No, this line has absolutely nothing to do with it. It's purely a comment that is not executed later. The line that has to be fixed is really the line starting with "extra_cflags=" in config-host.mak (well, at least in my experience - my patch does not touch the first line, at it still solves the issue). > Something like > > for arg in "$0" "$@"; do > quoted_arg=$(echo "$i" | sed 's/[$\\"]/\\&/g') > printf ' "%s"' "$quoted_arg" > done >> $config_host_mak But we could indeed apply that escaping mechanism to extra_cflags (or maybe to every variable that is printed to config-host.mk). That's what I meant when I refered to the following bashism: printf "extra_cflags=%q\n" "$extra_cflags" Unfortunately, %q is not portable and we probably need something along the lines of your proposal above (note that it doesn't handle "(" though, which is precisely the one causing an issue in my example).
Il 11/09/2013 17:01, Gabriel Kerneis ha scritto: > On Wed, Sep 11, 2013 at 04:53:35PM +0200, Paolo Bonzini wrote: >> Oh, then it's this line in configure that has to be changed to do proper >> quoting. >> >> printf "# Configured with:" >> $config_host_mak >> printf " '%s'" "$0" "$@" >> $config_host_mak > > No, this line has absolutely nothing to do with it. It's purely a > comment that is not executed later. Actually it is... :) See here: sed -n "/.*Configured with/s/[^:]*: //p" $@ | sh and relate it to your error message: config-host.mak is out-of-date, running configure sh: 1: Syntax error: "(" unexpected make: *** [config-host.mak] Error 2 Your argument is using '', so it conflicts with configure's own use of '' to quote the arguments. > The line that has to be fixed is > really the line starting with "extra_cflags=" in config-host.mak (well, > at least in my experience - my patch does not touch the first line, at > it still solves the issue). Yeah, what I'm missing now is why your patch works. > Unfortunately, %q is not portable and we probably need something along > the lines of your proposal above (note that it doesn't handle "(" > though, which is precisely the one causing an issue in my example). It doesn't need to handle it, because it is not a special character within quotes. Paolo
On Wed, Sep 11, 2013 at 04:01:37PM +0100, Gabriel Kerneis wrote: > On Wed, Sep 11, 2013 at 04:53:35PM +0200, Paolo Bonzini wrote: > > Oh, then it's this line in configure that has to be changed to do proper > > quoting. > > > > printf "# Configured with:" >> $config_host_mak > > printf " '%s'" "$0" "$@" >> $config_host_mak > > No, this line has absolutely nothing to do with it. I'm sorry, you are correct. I was confused because I did not try your proposal from a clean build directory, running into a chicken-and-egg issue. I'll propose an updated patch.
On Wed, Sep 11, 2013 at 05:06:41PM +0200, Paolo Bonzini wrote: > > The line that has to be fixed is > > really the line starting with "extra_cflags=" in config-host.mak (well, > > at least in my experience - my patch does not touch the first line, at > > it still solves the issue). > > Yeah, what I'm missing now is why your patch works. That is indeed very mysterious :-)
On Wed, Sep 11, 2013 at 04:16:28PM +0100, Gabriel Kerneis wrote: > > Yeah, what I'm missing now is why your patch works. > > That is indeed very mysterious :-) Okay, the answer is simple enough: it doesn't fix the issue at all. Long story short, I had a sed script to rewrite the faulty extra_cflags= line but I did not realize it *also* fixed the "Configured with:" line! Then turned that into a patch for QEMU, and didn't test it thoroughly enough, with the aforementioned chicken-and-egg issue. Oh well, please ignore this whole thread. I'll fix it properly tomorrow.
Il 11/09/2013 17:23, Gabriel Kerneis ha scritto: > On Wed, Sep 11, 2013 at 04:16:28PM +0100, Gabriel Kerneis wrote: >>> > > Yeah, what I'm missing now is why your patch works. >> > >> > That is indeed very mysterious :-) > Okay, the answer is simple enough: it doesn't fix the issue at all. > Long story short, I had a sed script to rewrite the faulty extra_cflags= > line but I did not realize it *also* fixed the "Configured with:" line! > Then turned that into a patch for QEMU, and didn't test it thoroughly > enough, with the aforementioned chicken-and-egg issue. > > Oh well, please ignore this whole thread. I'll fix it properly tomorrow. No problem at all, it happens. Shell is boring, let's go shopping. Paolo
On 09/11/2013 08:53 AM, Paolo Bonzini wrote: > > printf "# Configured with:" >> $config_host_mak > printf " '%s'" "$0" "$@" >> $config_host_mak > > Something like > > for arg in "$0" "$@"; do > quoted_arg=$(echo "$i" | sed 's/[$\\"]/\\&/g') Won't work as written: mismatch between $arg vs. $i. That missed ` - but do we expect anyone to pass a literal ` as one of their arguments? Also, it strips any literal trailing newlines in the arguments. > printf ' "%s"' "$quoted_arg" > done >> $config_host_mak > > could replace the second line. Adding Eric Blake in case he knows some > extra trick. Nope, no good portable tricks for this. But to address the (corner case) issues I mentioned above, it might be worth using '' rather than "" quoting; as well as embed a trailing space to guarantee no trailing newline: for arg in "$0" "$@"; do quoted_arg=$(echo "$arg " | sed "s/'/'\\''/g') printf "'%s'" "$quoted_arg" done >> $config_host_mak if you don't mind a trailing space in the output file.
diff --git a/configure b/configure index e989609..23114de 100755 --- a/configure +++ b/configure @@ -3681,7 +3681,7 @@ if test "$mingw32" = "no" ; then echo "qemu_localstatedir=$local_statedir" >> $config_host_mak fi echo "qemu_helperdir=$libexecdir" >> $config_host_mak -echo "extra_cflags=$EXTRA_CFLAGS" >> $config_host_mak +echo "extra_cflags=\"$EXTRA_CFLAGS\"" >> $config_host_mak echo "extra_ldflags=$EXTRA_LDFLAGS" >> $config_host_mak echo "qemu_localedir=$qemu_localedir" >> $config_host_mak echo "libs_softmmu=$libs_softmmu" >> $config_host_mak
The variable extra_cflags needs to be quoted in config-host.mak, in particular because it might contain parentheses that would otherwise be interpreted by the shell when reloading the file. For instance, if one wants to define some attribute with configure: ./configure --extra-cflags="-Dcoroutine_fn='__attribute__((coroutine_fn))'" A more robust approach would be to escape every variable properly, but there is no portable equivalent to bash's "printf %q" solution. The current patch, while not bullet-proof, works well in the common case. Signed-off-by: Gabriel Kerneis <gabriel@kerneis.info> --- configure | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)