diff mbox

[v2,4/4] char: convert qemu_chr_fe_write to qemu_chr_fe_write_all

Message ID 1473170165-540-5-git-send-email-berrange@redhat.com
State New
Headers show

Commit Message

Daniel P. Berrangé Sept. 6, 2016, 1:56 p.m. UTC
The mux chardev was not checking the return value of any
qemu_chr_fe_write() call so would silently loose data
on EAGAIN.

Similarly the qemu_chr_fe_printf method would not check
errors and was not in a position to retry even if it
could check.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Paolo Bonzini Sept. 6, 2016, 3:23 p.m. UTC | #1
On 06/09/2016 15:56, Daniel P. Berrange wrote:
> The mux chardev was not checking the return value of any
> qemu_chr_fe_write() call so would silently loose data
> on EAGAIN.
> 
> Similarly the qemu_chr_fe_printf method would not check
> errors and was not in a position to retry even if it
> could check.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-char.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 5f82ebb..6104f24 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -440,7 +440,9 @@ void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
>      va_list ap;
>      va_start(ap, fmt);
>      vsnprintf(buf, sizeof(buf), fmt, ap);
> -    qemu_chr_fe_write(s, (uint8_t *)buf, strlen(buf));
> +    /* XXX this blocks entire thread. Rewrite to use
> +     * qemu_chr_fe_write and background I/O callbacks */
> +    qemu_chr_fe_write_all(s, (uint8_t *)buf, strlen(buf));
>      va_end(ap);
>  }
>  
> @@ -556,7 +558,9 @@ static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
>                           (secs / 60) % 60,
>                           secs % 60,
>                           (int)(ti % 1000));
> -                qemu_chr_fe_write(d->drv, (uint8_t *)buf1, strlen(buf1));
> +                /* XXX this blocks entire thread. Rewrite to use
> +                 * qemu_chr_fe_write and background I/O callbacks */
> +                qemu_chr_fe_write_all(d->drv, (uint8_t *)buf1, strlen(buf1));
>                  d->linestart = 0;
>              }
>              ret += qemu_chr_fe_write(d->drv, buf+i, 1);
> @@ -594,13 +598,15 @@ static void mux_print_help(CharDriverState *chr)
>                   "\n\rEscape-Char set to Ascii: 0x%02x\n\r\n\r",
>                   term_escape_char);
>      }
> -    qemu_chr_fe_write(chr, (uint8_t *)cbuf, strlen(cbuf));
> +    /* XXX this blocks entire thread. Rewrite to use
> +     * qemu_chr_fe_write and background I/O callbacks */
> +    qemu_chr_fe_write_all(chr, (uint8_t *)cbuf, strlen(cbuf));
>      for (i = 0; mux_help[i] != NULL; i++) {
>          for (j=0; mux_help[i][j] != '\0'; j++) {
>              if (mux_help[i][j] == '%')
> -                qemu_chr_fe_write(chr, (uint8_t *)ebuf, strlen(ebuf));
> +                qemu_chr_fe_write_all(chr, (uint8_t *)ebuf, strlen(ebuf));
>              else
> -                qemu_chr_fe_write(chr, (uint8_t *)&mux_help[i][j], 1);
> +                qemu_chr_fe_write_all(chr, (uint8_t *)&mux_help[i][j], 1);
>          }
>      }
>  }
> @@ -625,7 +631,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
>          case 'x':
>              {
>                   const char *term =  "QEMU: Terminated\n\r";
> -                 qemu_chr_fe_write(chr, (uint8_t *)term, strlen(term));
> +                 qemu_chr_fe_write_all(chr, (uint8_t *)term, strlen(term));
>                   exit(0);
>                   break;
>              }
> 

Queued for 2.8, thanks.

Paolo
Eric Blake Sept. 8, 2016, 2:24 p.m. UTC | #2
On 09/06/2016 08:56 AM, Daniel P. Berrange wrote:
> The mux chardev was not checking the return value of any
> qemu_chr_fe_write() call so would silently loose data

s/loose/lose/

> on EAGAIN.
> 
> Similarly the qemu_chr_fe_printf method would not check
> errors and was not in a position to retry even if it
> could check.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-char.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
diff mbox

Patch

diff --git a/qemu-char.c b/qemu-char.c
index 5f82ebb..6104f24 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -440,7 +440,9 @@  void qemu_chr_fe_printf(CharDriverState *s, const char *fmt, ...)
     va_list ap;
     va_start(ap, fmt);
     vsnprintf(buf, sizeof(buf), fmt, ap);
-    qemu_chr_fe_write(s, (uint8_t *)buf, strlen(buf));
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(s, (uint8_t *)buf, strlen(buf));
     va_end(ap);
 }
 
@@ -556,7 +558,9 @@  static int mux_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
                          (secs / 60) % 60,
                          secs % 60,
                          (int)(ti % 1000));
-                qemu_chr_fe_write(d->drv, (uint8_t *)buf1, strlen(buf1));
+                /* XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks */
+                qemu_chr_fe_write_all(d->drv, (uint8_t *)buf1, strlen(buf1));
                 d->linestart = 0;
             }
             ret += qemu_chr_fe_write(d->drv, buf+i, 1);
@@ -594,13 +598,15 @@  static void mux_print_help(CharDriverState *chr)
                  "\n\rEscape-Char set to Ascii: 0x%02x\n\r\n\r",
                  term_escape_char);
     }
-    qemu_chr_fe_write(chr, (uint8_t *)cbuf, strlen(cbuf));
+    /* XXX this blocks entire thread. Rewrite to use
+     * qemu_chr_fe_write and background I/O callbacks */
+    qemu_chr_fe_write_all(chr, (uint8_t *)cbuf, strlen(cbuf));
     for (i = 0; mux_help[i] != NULL; i++) {
         for (j=0; mux_help[i][j] != '\0'; j++) {
             if (mux_help[i][j] == '%')
-                qemu_chr_fe_write(chr, (uint8_t *)ebuf, strlen(ebuf));
+                qemu_chr_fe_write_all(chr, (uint8_t *)ebuf, strlen(ebuf));
             else
-                qemu_chr_fe_write(chr, (uint8_t *)&mux_help[i][j], 1);
+                qemu_chr_fe_write_all(chr, (uint8_t *)&mux_help[i][j], 1);
         }
     }
 }
@@ -625,7 +631,7 @@  static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
         case 'x':
             {
                  const char *term =  "QEMU: Terminated\n\r";
-                 qemu_chr_fe_write(chr, (uint8_t *)term, strlen(term));
+                 qemu_chr_fe_write_all(chr, (uint8_t *)term, strlen(term));
                  exit(0);
                  break;
             }