[Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Gabriel Kerneis]

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(-)

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
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Gabriel Kerneis]

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.

-- 
Gabriel

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Gabriel Kerneis]

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).

-- 
Gabriel

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Gabriel Kerneis]

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 :-)

-- 
Gabriel

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Gabriel Kerneis]

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.

-- 
Gabriel

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Gabriel Kerneis]

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.

-- 
Gabriel

Re: [Qemu-devel] [PATCH] Quote extra_cflags in config-host.mak

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]