Re: [PATCH v4 12/20] libqtest: add in-process qtest.c tx/rx handlers

[Alexander Bulekov <alxndr@bu.edu>]

On 11/6/19 11:56 AM, Stefan Hajnoczi wrote:
> On Wed, Oct 30, 2019 at 02:49:58PM +0000, Oleinik, Alexander wrote:
>> From: Alexander Oleinik <alxndr@bu.edu>
>>
>> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
>> ---
>> There's a particularily ugly line here:
>> qtest_client_set_tx_handler(qts,
>>          (void (*)(QTestState *s, const char*, size_t)) send);
> 
> Please typedef the function pointer to avoid repetition:
> 
>    typedef void (*QTestSendFn)(QTestState *s, const char *buf, size_t len);
> 
> And then introduce a wrapper function for type-safety:
> 
>    /* A type-safe wrapper for s->send() */
>    static void send_wrapper(QTestState *s, const char *buf, size_t len)
>    {
>        s->send(s, buf, len);
>    }
> 
>    ...
> 
>    qts->send = send;
>    qtest_client_set_tx_handler(qts, send_wrapper);
> 
> Does this solve the issue?
So there should be two pointers qts->send and qts->ops->send? Otherwise 
qtest_client_set_tx_handler simply overwrites qts->send with the 
send_wrapper.

What I'm worried about is having to cast a
(void (*)(void *s, const char*, size_t) to a
(void (*)(QTestState *s, const char*, size_t)
I don't think this is defined according to the standard. If we add a 
secondary send function pointer to qts (void (*)(void *s, const char*, 
size_t)), then I think its no longer an issue, which I think is what you 
suggest above.

> By the way, I also wonder whether the size_t len arguments are necessary
> since const char *buf is a NUL-terminated C string.  The string should
> be enough since the length can be calculated from it.
I'll change it.

>> diff --git a/qtest.c b/qtest.c
>> index 9fbfa0f08f..f817a5d789 100644
>> --- a/qtest.c
>> +++ b/qtest.c
>> @@ -812,6 +812,6 @@ void qtest_server_inproc_recv(void *dummy, const char *buf, size_t size)
>>       g_string_append(gstr, buf);
>>       if (gstr->str[gstr->len - 1] == '\n') {
>>           qtest_process_inbuf(NULL, gstr);
>> -        g_string_free(gstr, true);
>> +        g_string_truncate(gstr, 0);
> 
> Ah, a fix for the bug in an earlier commit.  Please squash it.
> 
>> diff --git a/tests/libqtest.c b/tests/libqtest.c
>> index ff3153daf2..6143af33da 100644
>> --- a/tests/libqtest.c
>> +++ b/tests/libqtest.c
>> @@ -71,6 +71,7 @@ static void qtest_client_set_tx_handler(QTestState *s,
>>   static void qtest_client_set_rx_handler(QTestState *s,
>>           GString * (*recv)(QTestState *));
>>   
>> +static GString *recv_str;
> 
> Can this be a QTestState field?
> 


-- 
===
I recently changed my last name from Oleinik to Bulekov
===