Re: [PATCH v4 12/20] libqtest: add in-process qtest.c tx/rx handlers
On 11/6/19 11:56 AM, Stefan Hajnoczi wrote:
On Wed, Oct 30, 2019 at 02:49:58PM +, Oleinik, Alexander wrote:
From: Alexander Oleinik
Signed-off-by: Alexander Oleinik
---
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
===
Re: [PATCH v4 12/20] libqtest: add in-process qtest.c tx/rx handlers
On Wed, Oct 30, 2019 at 02:49:58PM +, Oleinik, Alexander wrote:
> From: Alexander Oleinik
>
> Signed-off-by: Alexander Oleinik
> ---
> 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?
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.
> 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?
signature.asc
Description: PGP signature
[PATCH v4 12/20] libqtest: add in-process qtest.c tx/rx handlers
From: Alexander Oleinik
Signed-off-by: Alexander Oleinik
---
There's a particularily ugly line here:
qtest_client_set_tx_handler(qts,
(void (*)(QTestState *s, const char*, size_t)) send);
Since qtest.c has no knowledge of the QTestState, I'm not sure how to
avoid doing this, without adding back the *opaque that was present in
v3.
qtest.c | 2 +-
tests/libqtest.c | 49
tests/libqtest.h | 5 +
3 files changed, 55 insertions(+), 1 deletion(-)
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);
}
}
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;
static int init_socket(const char *socket_path)
{
@@ -486,6 +487,7 @@ static GString *qtest_client_socket_recv_line(QTestState *s)
return line;
}
+
static gchar **qtest_rsp(QTestState *s, int expected_args)
{
GString *line;
@@ -1372,3 +1374,50 @@ static void qtest_client_set_rx_handler(QTestState *s,
{
s->ops.recv_line = recv;
}
+
+static GString *qtest_client_inproc_recv_line(QTestState *s)
+{
+GString *line;
+size_t offset;
+char *eol;
+
+eol = strchr(recv_str->str, '\n');
+offset = eol - recv_str->str;
+line = g_string_new_len(recv_str->str, offset);
+g_string_erase(recv_str, 0, offset + 1);
+return line;
+}
+
+QTestState *qtest_inproc_init(bool log, const char* arch,
+void (*send)(void*, const char*, size_t))
+{
+QTestState *qts;
+qts = g_new(QTestState, 1);
+qts->wstatus = 0;
+for (int i = 0; i < MAX_IRQ; i++) {
+qts->irq_level[i] = false;
+}
+
+qtest_client_set_rx_handler(qts, qtest_client_inproc_recv_line);
+/* Re-cast the send pointer, since qtest.c should need to know about
+ * QTestState
+ */
+qtest_client_set_tx_handler(qts,
+(void (*)(QTestState *s, const char*, size_t)) send);
+
+qts->big_endian = qtest_query_target_endianness(qts);
+gchar *bin_path = g_strconcat("/qemu-system-", arch, NULL);
+setenv("QTEST_QEMU_BINARY", bin_path, 0);
+g_free(bin_path);
+
+return qts;
+}
+
+void qtest_client_inproc_recv(void *opaque, const char *str, size_t len)
+{
+if (!recv_str) {
+recv_str = g_string_new(NULL);
+}
+g_string_append_len(recv_str, str, len);
+return;
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 31267fc915..7251de4ba9 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -728,4 +728,9 @@ bool qtest_probe_child(QTestState *s);
* Set expected exit status of the child.
*/
void qtest_set_expected_status(QTestState *s, int status);
+
+
+QTestState *qtest_inproc_init(bool log, const char* arch,
+void (*send)(void*, const char*, size_t));
+void qtest_client_inproc_recv(void *opaque, const char *str, size_t len);
#endif
--
2.23.0
