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

[Darren Kenny <darren.kenny@oracle.com>]

On Wed, Oct 30, 2019 at 02:49:51PM +0000, Oleinik, Alexander wrote:
>From: Alexander Oleinik <alxndr@bu.edu>
>
>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 <alxndr@bu.edu>
>---
> 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
>
>