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

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

[Andreas Färber]

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 <stefanha@redhat.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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' }");
+
     if (getenv("QTEST_STOP")) {
         kill(s->qemu_pid, SIGSTOP);
     }
-- 
1.8.4.5

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

[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 <stefanha@redhat.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  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.

Stefan

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 <stefanha@redhat.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  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

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

[Stefan Hajnoczi]

On Fri, Feb 14, 2014 at 4:38 PM, Andreas Färber <afaerber@suse.de> wrote:
> 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 <stefanha@redhat.com>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>  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?

I have an idea to get rid of the pid file entirely.  Will send a patch.

Stefan