Re: [Qemu-devel] [PATCH] qtest: Unlink pid file before reading from QMP

[Andreas Färber]

Am 14.02.2014 15:43, schrieb Stefan Hajnoczi:
> On Sun, Feb 09, 2014 at 12:21:41PM +0100, Andreas Färber wrote:
>> Despite 1ad3c6abc0d67e00b84abaa5527bc64b70ca2205, supplying invalid
>> arguments to the QEMU process still leaked a /tmp/qtest-*.pid file.
>>
>> Fix this by reordering the reading and unlinking to before reading from
>> QMP socket, which relies on a running process.
>>
>> Cc: Stefan Hajnoczi <address@hidden>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  tests/libqtest.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index c9a4f89..9433782 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -157,14 +157,14 @@ QTestState *qtest_init(const char *extra_args)
>>          s->irq_level[i] = false;
>>      }
>>  
>> -    /* Read the QMP greeting and then do the handshake */
>> -    qtest_qmp_discard_response(s, "");
>> -    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>> -
>>      s->qemu_pid = read_pid_file(pid_file);
>>      unlink(pid_file);
>>      g_free(pid_file);
>>  
>> +    /* Read the QMP greeting and then do the handshake */
>> +    qtest_qmp_discard_response(s, "");
>> +    qtest_qmp_discard_response(s, "{ 'execute': 'qmp_capabilities' }");
>> +
> 
> Hmm...the original ordering was intentional.
> 
> In order to avoid race conditions between QEMU creating the pid file and
> qtest reading the pid file, we wait until socket communication with QEMU
> has been performed.  That way we're sure the pid file already exists.
> 
> I think the tests can now fail due to the race condition.

I just verified that this patch did not slip into my pull. :)

Can you please propose an alternative solution or patch?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg