Re: [Qemu-devel] [PATCH 2/3] qtest: make QEMU our direct child process

[Markus Armbruster]

Stefan Hajnoczi <address@hidden> writes:

> On Mon, Feb 17, 2014 at 05:44:55PM +0100, Markus Armbruster wrote:
>> Stefan Hajnoczi <address@hidden> writes:
>> 
>> > qtest_init() cannot use exec*p() to launch QEMU since the exec*p()
>> > functions take an argument array while qtest_init() takes char
>> > *extra_args.  Therefore we execute /bin/sh -c <command-line> and let the
>> > shell parse the argument string.
>> >
>> > This left /bin/sh as our child process and our child's child was QEMU.
>> > We still want QEMU's pid so the -pidfile option was used to let QEMU
>> > report its pid.
>> >
>> > The pidfile needs to be unlinked when the test case exits or fails.  In
>> > other words, the pidfile creates a new problem for us!
>> >
>> > Simplify all this using the shell 'exec' command.  It allows us to
>> > replace the /bin/sh process with QEMU.  Then we no longer need to use
>> > -pidfile because we already know our fork child's pid.
>> >
>> > Note: Yes, it seems silly to exec /bin/sh when we could just exec QEMU
>> > directly.  But remember qtest_init() takes a single char *extra_args
>> > command-line fragment instead of a real argv[] array, so we need
>> > /bin/sh's argument parsing behavior.
>> 
>> Sounds like a design mistake to me.
>
> I wouldn't call char *extra_args a mistake because strings are still
> simpler to manipulate in C than char *argv[] arrays.  So we write less
> code in test cases and libqtest.c at the expense of a roundabout way to
> spawn the process.

Building command arguments in a string is deceptively simple.  The
deception collapses once you add funny characters that make the shell do
funny and unexpected things.

Our extra_args string manipulations are typically of the form "format a
bunch of options and option arguments into a string".  I suspect using a
function to collect a bunch of options and option arguments into an
array would be about the same amount of code in most cases.

Just sayin'; I'm not objecting to your patch.