Re: [PATCH v4 04/20] qtest: add qtest_server_send abstraction

[Darren Kenny]

On Wed, Oct 30, 2019 at 02:49:51PM +0000, Oleinik, Alexander wrote:
> From: Alexander Oleinik <address@hidden>
>
> qtest_server_send is a function pointer specifying the handler used to
> transmit data to the qtest client. In the standard configuration, this
> calls the CharBackend handler, but now it is possible for other types of
> handlers, e.g direct-function calls if the qtest client and server
> exist within the same process (inproc)
>
> Signed-off-by: Alexander Oleinik <address@hidden>
> ---
> include/sysemu/qtest.h |  3 +++
> qtest.c                | 17 +++++++++++++++--
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/include/sysemu/qtest.h b/include/sysemu/qtest.h
> index 5ed09c80b1..fda7000d2c 100644
> --- a/include/sysemu/qtest.h
> +++ b/include/sysemu/qtest.h
> @@ -26,4 +26,7 @@ bool qtest_driver(void);
>
> void qtest_server_init(const char *qtest_chrdev, const char *qtest_log, Error 
> **errp);
>
> +void qtest_server_set_tx_handler(void (*send)(void *, const char *, size_t),
> +                                 void *opaque);
> +
> #endif
> diff --git a/qtest.c b/qtest.c
> index 8b50e2783e..ae7e6d779d 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -42,6 +42,8 @@ static GString *inbuf;
> static int irq_levels[MAX_IRQ];
> static qemu_timeval start_time;
> static bool qtest_opened;
> +static void (*qtest_server_send)(void*, const char*, size_t);
> +static void *qtest_server_send_opaque;
>
> #define FMT_timeval "%ld.%06ld"
>
> @@ -228,8 +230,9 @@ static void GCC_FMT_ATTR(1, 2) qtest_log_send(const char 
> *fmt, ...)
>     va_end(ap);
> }
>
> -static void do_qtest_send(CharBackend *chr, const char *str, size_t len)
> +static void qtest_server_char_be_send(void *opaque, const char *str, size_t 
> len)
> {
> +    CharBackend* chr = (CharBackend *)opaque;
>     qemu_chr_fe_write_all(chr, (uint8_t *)str, len);
>     if (qtest_log_fp && qtest_opened) {
>         fprintf(qtest_log_fp, "%s", str);
> @@ -238,7 +241,7 @@ static void do_qtest_send(CharBackend *chr, const char 
> *str, size_t len)
>
> static void qtest_send(CharBackend *chr, const char *str)
> {
> -    do_qtest_send(chr, str, strlen(str));

Given that this is a function pointer, it may be worth asserting
that it is not NULL.

> +    qtest_server_send(qtest_server_send_opaque, str, strlen(str));
> }
>
> static void GCC_FMT_ATTR(2, 3) qtest_sendf(CharBackend *chr,
> @@ -783,6 +786,16 @@ void qtest_server_init(const char *qtest_chrdev, const 
> char *qtest_log, Error **
>     qemu_chr_fe_set_echo(&qtest_chr, true);
>
>     inbuf = g_string_new("");
> +
> +    if (!qtest_server_send) {
> +        qtest_server_set_tx_handler(qtest_server_char_be_send, &qtest_chr);
> +    }
> +}
> +
> +void qtest_server_set_tx_handler(void (*send)(void*, const char*, size_t), 
> void *opaque)

It's a nit, but it might be better to rename this to match the
variables actually being set, i.e. s/_tx_/_send_/.

Thanks,

Darren.

> +{
> +    qtest_server_send = send;
> +    qtest_server_send_opaque = opaque;
> }
>
> bool qtest_driver(void)
> --
> 2.23.0