diff mbox series

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

Message ID 20191030144926.11873-13-alxndr@bu.edu
State New
Headers show
Series Add virtual device fuzzing support | expand

Commit Message

Alexander Bulekov Oct. 30, 2019, 2:49 p.m. UTC 
From: Alexander Oleinik <alxndr@bu.edu>

Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
---
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(-)

Comments

Stefan Hajnoczi Nov. 6, 2019, 4:56 p.m. UTC | #1 
On Wed, Oct 30, 2019 at 02:49:58PM +0000, Oleinik, Alexander wrote:
> From: Alexander Oleinik <alxndr@bu.edu>
> 
> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
> ---
> 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?
Alexander Bulekov Nov. 12, 2019, 5:38 p.m. UTC | #2 
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 <alxndr@bu.edu>
>>
>> Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
>> ---
>> 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?
>
diff mbox series

Patch

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