Re: [PATCH V3 13/16] tests/qtest: defer connection

[Steven Sistare <steven.sistare@oracle.com>]

On 11/13/2024 5:53 PM, Peter Xu wrote:
> On Fri, Nov 01, 2024 at 06:47:52AM -0700, Steve Sistare wrote:
>> +void qtest_connect_deferred(QTestState *s)
>> +{
>> +    g_autofree gchar *socket_path = NULL;
>> +    g_autofree gchar *qmp_socket_path = NULL;
>> +
>> +    socket_path = g_strdup_printf("%s/qtest-%d.sock",
>> +                                  g_get_tmp_dir(), getpid());
>> +    qmp_socket_path = g_strdup_printf("%s/qtest-%d.qmp",
>> +                                      g_get_tmp_dir(), getpid());
>> +
>> +    s->fd = socket_accept(s->sock);
>> +    if (s->fd >= 0) {
>> +        s->qmp_fd = socket_accept(s->qmpsock);
>> +    }
>> +    unlink(socket_path);
>> +    unlink(qmp_socket_path);
> 
> Why need to unlink again here if both sock/qmpsock are cached?  I assume we
> could remove these lines together with above g_strdup_printf()s.
> 
> Otherwise two paths are leaked anyway (and we may also want to have some
> macros to represent the paths used in two places).

The original code in qtest_init_internal unlinked before creating the socket, as
a precaution, and after accepting. I assume the latter for cleanliness.  I
carried that forward.

I'll define a helper function to eliminate the format string duplication, and
I'll fix the pre-existing leak.

static char *qtest_socket_path(const char *suffix)
{
     return g_strdup_printf("%s/qtest-%d.%s", g_get_tmp_dir(), getpid(), suffix)
}

qtest_init_internal()
     g_autofree gchar *socket_path = qtest_socket_path("sock");
     g_autofree gchar *qmp_socket_path = qtest_socket_path("qmp");

> Maybe we could also clear sock/qmpsock too after use, then check at the
> entrance to skip qtest_connect_deferred() if already connected.

Will do.

- Steve

>> +    g_assert(s->fd >= 0 && s->qmp_fd >= 0);
>>       /* ask endianness of the target */
>> -
>>       s->big_endian = qtest_query_target_endianness(s);
>> -
>> -   return s;
>>   }
>