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

[Alexander Bulekov]

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 <address@hidden>
> >
> > Signed-off-by: Alexander Oleinik <address@hidden>
> > ---
> > 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
===