Re: [PATCH v6 05/21] libqtest: Add a layer of abstraciton to send/recv

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Aleksander,

Typo in patch subject: "abstraction"

On 11/29/19 10:34 PM, Oleinik, Alexander wrote:
> This makes it simple to swap the transport functions for qtest commands
> to and from the qtest client. For example, now it is possible to
> directly pass qtest commands to a server handler that exists within the
> same process, without the standard way of writing to a file descriptor.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>   tests/libqtest.c | 50 +++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index 91e9cb220c..ac4b6ab5f0 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -35,6 +35,17 @@
>   #define SOCKET_TIMEOUT 50
>   #define SOCKET_MAX_FDS 16
>   
> +
> +typedef void (*QTestSendFn)(QTestState *s, const char *buf);
> +typedef void (*ExternalSendFn)(void *s, const char *buf);
> +typedef GString* (*QTestRecvFn)(QTestState *);
> +
> +typedef struct QTestClientTransportOps {
> +    QTestSendFn     send;

We use strlen() so we also send a send a string. Maybe rename it send_line?

Please add a comment here about what does each function handler in this 
structure.

> +    ExternalSendFn  external_send;

Is external_send used? Apparently in patch #12. Can you add it there, 
where it is used? I don't understand why we a "type-safe wrapper" is 
called 'external'.

> +    QTestRecvFn     recv_line;
> +} QTestTransportOps;
> +
>   struct QTestState
>   {
>       int fd;
> @@ -45,6 +56,7 @@ struct QTestState
>       bool big_endian;
>       bool irq_level[MAX_IRQ];
>       GString *rx;
> +    QTestTransportOps ops;
>   };
>   
>   static GHookList abrt_hooks;
> @@ -52,6 +64,14 @@ static struct sigaction sigact_old;
>   
>   static int qtest_query_target_endianness(QTestState *s);
>   
> +static void qtest_client_socket_send(QTestState*, const char *buf);
> +static void socket_send(int fd, const char *buf, size_t size);
> +
> +static GString *qtest_client_socket_recv_line(QTestState *);
> +
> +static void qtest_client_set_tx_handler(QTestState *s, QTestSendFn send);
> +static void qtest_client_set_rx_handler(QTestState *s, QTestRecvFn recv);
> +
>   static int init_socket(const char *socket_path)
>   {
>       struct sockaddr_un addr;
> @@ -234,6 +254,9 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>       sock = init_socket(socket_path);
>       qmpsock = init_socket(qmp_socket_path);
>   
> +    qtest_client_set_rx_handler(s, qtest_client_socket_recv_line);
> +    qtest_client_set_tx_handler(s, qtest_client_socket_send);
> +
>       qtest_add_abrt_handler(kill_qemu_hook_func, s);
>   
>       command = g_strdup_printf("exec %s "
> @@ -379,13 +402,9 @@ static void socket_send(int fd, const char *buf, size_t size)
>       }
>   }
>   
> -static void socket_sendf(int fd, const char *fmt, va_list ap)
> +static void qtest_client_socket_send(QTestState *s, const char *buf)
>   {
> -    gchar *str = g_strdup_vprintf(fmt, ap);
> -    size_t size = strlen(str);
> -
> -    socket_send(fd, str, size);
> -    g_free(str);
> +    socket_send(s->fd, buf, strlen(buf));
>   }
>   
>   static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
> @@ -393,8 +412,11 @@ static void GCC_FMT_ATTR(2, 3) qtest_sendf(QTestState *s, const char *fmt, ...)
>       va_list ap;
>   
>       va_start(ap, fmt);
> -    socket_sendf(s->fd, fmt, ap);
> +    gchar *str = g_strdup_vprintf(fmt, ap);
>       va_end(ap);
> +
> +    s->ops.send(s, str);
> +    g_free(str);
>   }
>   
>   /* Sends a message and file descriptors to the socket.
> @@ -431,7 +453,7 @@ static void socket_send_fds(int socket_fd, int *fds, size_t fds_num,
>       g_assert_cmpint(ret, >, 0);
>   }
>   
> -static GString *qtest_recv_line(QTestState *s)
> +static GString *qtest_client_socket_recv_line(QTestState *s)
>   {
>       GString *line;
>       size_t offset;
> @@ -468,7 +490,7 @@ static gchar **qtest_rsp(QTestState *s, int expected_args)
>       int i;
>   
>   redo:
> -    line = qtest_recv_line(s);
> +    line = s->ops.recv_line(s);
>       words = g_strsplit(line->str, " ", 0);
>       g_string_free(line, TRUE);
>   
> @@ -1337,3 +1359,13 @@ void qmp_assert_error_class(QDict *rsp, const char *class)
>   
>       qobject_unref(rsp);
>   }
> +
> +static void qtest_client_set_tx_handler(QTestState *s,
> +                    QTestSendFn send)
> +{
> +    s->ops.send = send;
> +}
> +static void qtest_client_set_rx_handler(QTestState *s, QTestRecvFn recv)
> +{
> +    s->ops.recv_line = recv;
> +}
>