[PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Thomas Huth]

g_test_rand_int() must not be called before g_test_init(), otherwise
the glib will show a "g_rand_int: assertion 'rand != NULL' failed"
message in the log. So we could change the order here, but actually,
g_test_rand_int() seems also the wrong choice here, since we're using
it to create a temporary file name, which certainly should not depend
on the "--seed" CLI option of the g_test suite. Thus let's use the
g_random_int() function instead.

Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qtest/virtio-net-failover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
index 4b2ba8a106..70b94bf18f 100644
--- a/tests/qtest/virtio-net-failover.c
+++ b/tests/qtest/virtio-net-failover.c
@@ -1308,7 +1308,7 @@ int main(int argc, char **argv)
 {
     const gchar *tmpdir = g_get_tmp_dir();
     gchar *tmpfile = g_strdup_printf("%s/failover_test_migrate-%u-%u",
-                                     tmpdir, getpid(), g_test_rand_int());
+                                     tmpdir, getpid(), g_random_int());
     int ret;
 
     g_test_init(&argc, &argv, NULL);
-- 
2.27.0

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Laurent Vivier]

On 20/12/2021 11:27, Thomas Huth wrote:
> g_test_rand_int() must not be called before g_test_init(), otherwise
> the glib will show a "g_rand_int: assertion 'rand != NULL' failed"
> message in the log. So we could change the order here, but actually,
> g_test_rand_int() seems also the wrong choice here, since we're using
> it to create a temporary file name, which certainly should not depend
> on the "--seed" CLI option of the g_test suite. Thus let's use the
> g_random_int() function instead.
>
> Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/qtest/virtio-net-failover.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/virtio-net-failover.c b/tests/qtest/virtio-net-failover.c
> index 4b2ba8a106..70b94bf18f 100644
> --- a/tests/qtest/virtio-net-failover.c
> +++ b/tests/qtest/virtio-net-failover.c
> @@ -1308,7 +1308,7 @@ int main(int argc, char **argv)
>   {
>       const gchar *tmpdir = g_get_tmp_dir();
>       gchar *tmpfile = g_strdup_printf("%s/failover_test_migrate-%u-%u",
> -                                     tmpdir, getpid(), g_test_rand_int());
> +                                     tmpdir, getpid(), g_random_int());
>       int ret;
>   
>       g_test_init(&argc, &argv, NULL);

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Philippe Mathieu-Daudé]

On 12/20/21 11:27, Thomas Huth wrote:
> g_test_rand_int() must not be called before g_test_init(), otherwise
> the glib will show a "g_rand_int: assertion 'rand != NULL' failed"
> message in the log. So we could change the order here, but actually,
> g_test_rand_int() seems also the wrong choice here, since we're using
> it to create a temporary file name, which certainly should not depend
> on the "--seed" CLI option of the g_test suite. Thus let's use the
> g_random_int() function instead.
> 
> Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/qtest/virtio-net-failover.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Richard Henderson]

On 12/20/21 2:27 AM, Thomas Huth wrote:
>       const gchar *tmpdir = g_get_tmp_dir();
>       gchar *tmpfile = g_strdup_printf("%s/failover_test_migrate-%u-%u",
> -                                     tmpdir, getpid(), g_test_rand_int());
> +                                     tmpdir, getpid(), g_random_int());

Random numbers plus pid are irrelevant, because you still don't have guaranteed uniqueness 
-- think stale files in /tmp.

Use g_file_open_tmp.


r~

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Philippe Mathieu-Daudé]

On 12/20/21 20:26, Richard Henderson wrote:
> On 12/20/21 2:27 AM, Thomas Huth wrote:
>>       const gchar *tmpdir = g_get_tmp_dir();
>>       gchar *tmpfile = g_strdup_printf("%s/failover_test_migrate-%u-%u",
>> -                                     tmpdir, getpid(),
>> g_test_rand_int());
>> +                                     tmpdir, getpid(), g_random_int());
> 
> Random numbers plus pid are irrelevant, because you still don't have
> guaranteed uniqueness -- think stale files in /tmp.
> 
> Use g_file_open_tmp.

Another use in test_socket_unix_abstract(),
tests/unit/test-util-sockets.c.

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Thomas Huth]

On 20/12/2021 21.02, Philippe Mathieu-Daudé wrote:
> On 12/20/21 20:26, Richard Henderson wrote:
>> On 12/20/21 2:27 AM, Thomas Huth wrote:
>>>        const gchar *tmpdir = g_get_tmp_dir();
>>>        gchar *tmpfile = g_strdup_printf("%s/failover_test_migrate-%u-%u",
>>> -                                     tmpdir, getpid(),
>>> g_test_rand_int());
>>> +                                     tmpdir, getpid(), g_random_int());
>>
>> Random numbers plus pid are irrelevant, because you still don't have
>> guaranteed uniqueness -- think stale files in /tmp.
>>
>> Use g_file_open_tmp.
> 
> Another use in test_socket_unix_abstract(),
> tests/unit/test-util-sockets.c.

Using g_file_open_tmp is certainly better ... but the tests are currently 
written in a way where they require the file name of the temporary file - so 
switching to g_file_open_tmp() (which only provides a file handle) certainly 
would need some rewrite here... Thus I'd suggest to go first with this patch 
to silence the Assert messages, and then to clean this up properly later.

  Thomas

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Richard Henderson]

On 12/21/21 2:32 AM, Thomas Huth wrote:
> Using g_file_open_tmp is certainly better ... but the tests are currently written in a way 
> where they require the file name of the temporary file - so switching to g_file_open_tmp() 
> (which only provides a file handle) certainly would need some rewrite here...

Incorrect.  g_file_open_tmp returns the open file handle, but also returns the filename in 
*name_used.

I should think you can close the file handle straight away and use the filename.


r~

Re: [PATCH] tests/qtest/virtio-net-failover: Use g_random_int() instead of g_test_rand_int()

[Thomas Huth]

On 21/12/2021 20.32, Richard Henderson wrote:
> On 12/21/21 2:32 AM, Thomas Huth wrote:
>> Using g_file_open_tmp is certainly better ... but the tests are currently 
>> written in a way where they require the file name of the temporary file - 
>> so switching to g_file_open_tmp() (which only provides a file handle) 
>> certainly would need some rewrite here...
> 
> Incorrect.  g_file_open_tmp returns the open file handle, but also returns 
> the filename in *name_used.

D'oh - not sure how I missed that yesterday :-/

> I should think you can close the file handle straight away and use the 
> filename.

Ok, then let's continue with that approach.

  Thomas