diff mbox

[v3,1/5] qtest: allow arbitrarily long sends

Message ID 1430864578-22072-2-git-send-email-jsnow@redhat.com
State New
Headers show

Commit Message

John Snow May 5, 2015, 10:22 p.m. UTC 
qtest currently has a static buffer of size 1024 that if we
overflow, ignores the additional data silently which leads
to hangs or stream failures.

Use glib's string facilities to allow arbitrarily long data,
but split this off into a new function, qtest_sendf.

Static data can still be sent using qtest_send, which avoids
the malloc/copy overflow.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qtest.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

Comments

Eric Blake May 5, 2015, 11:22 p.m. UTC | #1 
On 05/05/2015 04:22 PM, John Snow wrote:
> qtest currently has a static buffer of size 1024 that if we
> overflow, ignores the additional data silently which leads
> to hangs or stream failures.
> 
> Use glib's string facilities to allow arbitrarily long data,
> but split this off into a new function, qtest_sendf.
> 
> Static data can still be sent using qtest_send, which avoids
> the malloc/copy overflow.

Did you mean 'overhead' instead of 'overflow'?

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qtest.c | 46 ++++++++++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 18 deletions(-)
> 

>          qtest_send_prefix(chr);
> -        qtest_send(chr, "FAIL Unknown command `%s'\n", words[0]);
> +        qtest_sendf(chr, "FAIL Unknown command `%s'\n", words[0]);
>      }

Unrelated - these days, I don't see `this' quoting much any more (except
in m4); most people have moved to 'this' quoting.  We could clean it
while touching this line, but it really doesn't affect correctness
either way.

Reviewed-by: Eric Blake <eblake@redhat.com>
John Snow May 5, 2015, 11:35 p.m. UTC | #2 
On 05/05/2015 07:22 PM, Eric Blake wrote:
> On 05/05/2015 04:22 PM, John Snow wrote:
>> qtest currently has a static buffer of size 1024 that if we 
>> overflow, ignores the additional data silently which leads to
>> hangs or stream failures.
>> 
>> Use glib's string facilities to allow arbitrarily long data, but
>> split this off into a new function, qtest_sendf.
>> 
>> Static data can still be sent using qtest_send, which avoids the
>> malloc/copy overflow.
> 
> Did you mean 'overhead' instead of 'overflow'?
> 

Sigh, yes. The fingers run on autopilot without input from the brain.
Will nab it if I spin again.

>> 
>> Signed-off-by: John Snow <jsnow@redhat.com> --- qtest.c | 46
>> ++++++++++++++++++++++++++++------------------ 1 file changed, 28
>> insertions(+), 18 deletions(-)
>> 
> 
>> qtest_send_prefix(chr); -        qtest_send(chr, "FAIL Unknown
>> command `%s'\n", words[0]); +        qtest_sendf(chr, "FAIL
>> Unknown command `%s'\n", words[0]); }
> 
> Unrelated - these days, I don't see `this' quoting much any more
> (except in m4); most people have moved to 'this' quoting.  We could
> clean it while touching this line, but it really doesn't affect
> correctness either way.
> 

If I spin again I'll get it.

> Reviewed-by: Eric Blake <eblake@redhat.com>
>
diff mbox

Patch

diff --git a/qtest.c b/qtest.c
index 8d1e66c..f635a48 100644
--- a/qtest.c
+++ b/qtest.c
@@ -182,21 +182,29 @@  static void qtest_send_prefix(CharDriverState *chr)
             (long) tv.tv_sec, (long) tv.tv_usec);
 }
 
-static void GCC_FMT_ATTR(2, 3) qtest_send(CharDriverState *chr,
-                                          const char *fmt, ...)
+static void do_qtest_send(CharDriverState *chr, const char *str, size_t len)
+{
+    qemu_chr_fe_write_all(chr, (uint8_t *)str, len);
+    if (qtest_log_fp && qtest_opened) {
+        fprintf(qtest_log_fp, "%s", str);
+    }
+}
+
+static void qtest_send(CharDriverState *chr, const char *str)
+{
+    do_qtest_send(chr, str, strlen(str));
+}
+
+static void GCC_FMT_ATTR(2, 3) qtest_sendf(CharDriverState *chr,
+                                           const char *fmt, ...)
 {
     va_list ap;
-    char buffer[1024];
-    size_t len;
+    gchar *buffer;
 
     va_start(ap, fmt);
-    len = vsnprintf(buffer, sizeof(buffer), fmt, ap);
+    buffer = g_strdup_vprintf(fmt, ap);
+    qtest_send(chr, buffer);
     va_end(ap);
-
-    qemu_chr_fe_write_all(chr, (uint8_t *)buffer, len);
-    if (qtest_log_fp && qtest_opened) {
-        fprintf(qtest_log_fp, "%s", buffer);
-    }
 }
 
 static void qtest_irq_handler(void *opaque, int n, int level)
@@ -208,8 +216,8 @@  static void qtest_irq_handler(void *opaque, int n, int level)
         CharDriverState *chr = qtest_chr;
         irq_levels[n] = level;
         qtest_send_prefix(chr);
-        qtest_send(chr, "IRQ %s %d\n",
-                   level ? "raise" : "lower", n);
+        qtest_sendf(chr, "IRQ %s %d\n",
+                    level ? "raise" : "lower", n);
     }
 }
 
@@ -318,7 +326,7 @@  static void qtest_process_command(CharDriverState *chr, gchar **words)
             value = cpu_inl(addr);
         }
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK 0x%04x\n", value);
+        qtest_sendf(chr, "OK 0x%04x\n", value);
     } else if (strcmp(words[0], "writeb") == 0 ||
                strcmp(words[0], "writew") == 0 ||
                strcmp(words[0], "writel") == 0 ||
@@ -375,7 +383,7 @@  static void qtest_process_command(CharDriverState *chr, gchar **words)
             tswap64s(&value);
         }
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK 0x%016" PRIx64 "\n", value);
+        qtest_sendf(chr, "OK 0x%016" PRIx64 "\n", value);
     } else if (strcmp(words[0], "read") == 0) {
         uint64_t addr, len, i;
         uint8_t *data;
@@ -390,7 +398,7 @@  static void qtest_process_command(CharDriverState *chr, gchar **words)
         qtest_send_prefix(chr);
         qtest_send(chr, "OK 0x");
         for (i = 0; i < len; i++) {
-            qtest_send(chr, "%02x", data[i]);
+            qtest_sendf(chr, "%02x", data[i]);
         }
         qtest_send(chr, "\n");
 
@@ -434,7 +442,8 @@  static void qtest_process_command(CharDriverState *chr, gchar **words)
         }
         qtest_clock_warp(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns);
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK %"PRIi64"\n", (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        qtest_sendf(chr, "OK %"PRIi64"\n",
+                    (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     } else if (qtest_enabled() && strcmp(words[0], "clock_set") == 0) {
         int64_t ns;
 
@@ -442,10 +451,11 @@  static void qtest_process_command(CharDriverState *chr, gchar **words)
         ns = strtoll(words[1], NULL, 0);
         qtest_clock_warp(ns);
         qtest_send_prefix(chr);
-        qtest_send(chr, "OK %"PRIi64"\n", (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+        qtest_sendf(chr, "OK %"PRIi64"\n",
+                    (int64_t)qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     } else {
         qtest_send_prefix(chr);
-        qtest_send(chr, "FAIL Unknown command `%s'\n", words[0]);
+        qtest_sendf(chr, "FAIL Unknown command `%s'\n", words[0]);
     }
 }