[Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 include/char/char.h | 15 +++++++++++++++
 qemu-char.c         | 27 +++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/include/char/char.h b/include/char/char.h
index 0326b2a..5c3a7a5 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -170,6 +170,21 @@ int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
 int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len);
 
 /**
+ * @qemu_chr_fe_write_all:
+ *
+ * Write data to a character backend from the front end.  This function will
+ * send data from the front end to the back end.  Unlike @qemu_chr_fe_write,
+ * this function will block if the back end cannot consume all of the data
+ * attempted to be written.
+ *
+ * @buf the data
+ * @len the number of bytes to send
+ *
+ * Returns: the number of bytes consumed
+ */
+int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len);
+
+/**
  * @qemu_chr_fe_ioctl:
  *
  * Issue a device specific ioctl to a backend.
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..936150f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -140,6 +140,33 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len)
     return s->chr_write(s, buf, len);
 }
 
+int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)
+{
+    int offset = 0;
+    int res;
+
+    while (offset < len) {
+        do {
+            res = s->chr_write(s, buf + offset, len - offset);
+            if (res == -1 && errno == EAGAIN) {
+                g_usleep(100);
+            }
+        } while (res == -1 && errno == EAGAIN);
+
+        if (res == 0) {
+            break;
+        }
+
+        if (res < 0) {
+            return res;
+        }
+
+        offset += res;
+    }
+
+    return offset;
+}
+
 int qemu_chr_fe_ioctl(CharDriverState *s, int cmd, void *arg)
 {
     if (!s->chr_ioctl)
-- 
1.8.0

[Qemu-devel] [PATCH 2/2] qtest: use synchronous I/O for char device

[Anthony Liguori]

Peter reported that rtc-test would periodically hang.  It turns out
this was due to an EAGAIN occurring on qemu_chr_fe_write.

Instead of heavily refactoring qtest, just use a synchronous version
of the write operation for qemu_chr_fe_write to address this problem.

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 qtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qtest.c b/qtest.c
index 5e0e9ec..b03b68a 100644
--- a/qtest.c
+++ b/qtest.c
@@ -191,7 +191,7 @@ static void GCC_FMT_ATTR(2, 3) qtest_send(CharDriverState *chr,
     len = vsnprintf(buffer, sizeof(buffer), fmt, ap);
     va_end(ap);
 
-    qemu_chr_fe_write(chr, (uint8_t *)buffer, len);
+    qemu_chr_fe_write_all(chr, (uint8_t *)buffer, len);
     if (qtest_log_fp && qtest_opened) {
         fprintf(qtest_log_fp, "%s", buffer);
     }
-- 
1.8.0

Re: [Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Peter Maydell]

On 26 March 2013 15:11, Anthony Liguori <aliguori@us.ibm.com> wrote:
> +int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)
> +{
> +    int offset = 0;
> +    int res;
> +
> +    while (offset < len) {
> +        do {
> +            res = s->chr_write(s, buf + offset, len - offset);
> +            if (res == -1 && errno == EAGAIN) {
> +                g_usleep(100);
> +            }
> +        } while (res == -1 && errno == EAGAIN);

   for (;;) {
       res = s->chr_write(s, buf + offset, len - offset);
       if (!(res == -1 && errno == EAGAIN)) {
           break;
       }
       g_usleep(100);
   }

would avoid the duplication of the retry condition.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Wenchao Xia]

于 2013-3-26 23:21, Peter Maydell 写道:
> On 26 March 2013 15:11, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> +int qemu_chr_fe_write_all(CharDriverState *s, const uint8_t *buf, int len)
>> +{
>> +    int offset = 0;
>> +    int res;
>> +
>> +    while (offset < len) {
>> +        do {
>> +            res = s->chr_write(s, buf + offset, len - offset);
>> +            if (res == -1 && errno == EAGAIN) {
>> +                g_usleep(100);
>> +            }
>> +        } while (res == -1 && errno == EAGAIN);
>
>     for (;;) {
>         res = s->chr_write(s, buf + offset, len - offset);
>         if (!(res == -1 && errno == EAGAIN)) {
>             break;
>         }
>         g_usleep(100);
>     }
>
> would avoid the duplication of the retry condition.
>
> -- PMM
>
I think adjust like following make code easier to read:

     while (offset < len) {
         res = s->chr_write(s, buf + offset, len - offset);
         if (res == -1 && errno == EAGAIN) {
                 g_usleep(100)
                 continue;
         }
         .....
     }
-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 2/2] qtest: use synchronous I/O for char device

[Wenchao Xia]

Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

> Peter reported that rtc-test would periodically hang.  It turns out
> this was due to an EAGAIN occurring on qemu_chr_fe_write.
> 
> Instead of heavily refactoring qtest, just use a synchronous version
> of the write operation for qemu_chr_fe_write to address this problem.
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>   qtest.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qtest.c b/qtest.c
> index 5e0e9ec..b03b68a 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -191,7 +191,7 @@ static void GCC_FMT_ATTR(2, 3) qtest_send(CharDriverState *chr,
>       len = vsnprintf(buffer, sizeof(buffer), fmt, ap);
>       va_end(ap);
> 
> -    qemu_chr_fe_write(chr, (uint8_t *)buffer, len);
> +    qemu_chr_fe_write_all(chr, (uint8_t *)buffer, len);
>       if (qtest_log_fp && qtest_opened) {
>           fprintf(qtest_log_fp, "%s", buffer);
>       }
> 


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Peter Maydell]

On 27 March 2013 21:15, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Applied.  Thanks.

Replied to wrong email by accident, or applied ignoring
the review comments?

-- PMM

Re: [Qemu-devel] [PATCH 1/2] char: introduce a blocking version of qemu_chr_fe_write

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 27 March 2013 21:15, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Applied.  Thanks.
>
> Replied to wrong email by accident, or applied ignoring
> the review comments?

I interpreted your comment as a suggestion.  I'm not a fan of while
(true) loops so I left it as is.  Since it's a pretty important bug fix
(make check was failing), I tried to get it applied quickly.  I should
have responded to your mail before applying it though.

Regards,

Anthony Liguori

>
> -- PMM