[PATCH 0/3] ui/console: chardev backend improvements

[PATCH 0/3] ui/console: chardev backend improvements

[Volker Rümelin]

A few things I learnt while writing a fix for a chardev bug
in the GTK backend.

Volker Rümelin (3):
   ui/console: replace QEMUFIFO with Fifo8
   ui/console: replace kbd_timer with chr_accept_input callback
   ui/console: remove chardev frontend connected test

  ui/console.c | 109 ++++++++++++++-------------------------------------
  1 file changed, 29 insertions(+), 80 deletions(-)

-- 
2.31.1

[PATCH 1/3] ui/console: replace QEMUFIFO with Fifo8

[Volker Rümelin]

One of the two FIFO implementations QEMUFIFO and Fifo8 is
redundant. Replace QEMUFIFO with Fifo8.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 ui/console.c | 86 ++++++++++++----------------------------------------
 1 file changed, 20 insertions(+), 66 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index eabbbc951c..e6ce29024c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -27,6 +27,7 @@
 #include "hw/qdev-core.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-ui.h"
+#include "qemu/fifo8.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/timer.h"
@@ -62,57 +63,6 @@ enum TTYState {
     TTY_STATE_CSI,
 };
 
-typedef struct QEMUFIFO {
-    uint8_t *buf;
-    int buf_size;
-    int count, wptr, rptr;
-} QEMUFIFO;
-
-static int qemu_fifo_write(QEMUFIFO *f, const uint8_t *buf, int len1)
-{
-    int l, len;
-
-    l = f->buf_size - f->count;
-    if (len1 > l)
-        len1 = l;
-    len = len1;
-    while (len > 0) {
-        l = f->buf_size - f->wptr;
-        if (l > len)
-            l = len;
-        memcpy(f->buf + f->wptr, buf, l);
-        f->wptr += l;
-        if (f->wptr >= f->buf_size)
-            f->wptr = 0;
-        buf += l;
-        len -= l;
-    }
-    f->count += len1;
-    return len1;
-}
-
-static int qemu_fifo_read(QEMUFIFO *f, uint8_t *buf, int len1)
-{
-    int l, len;
-
-    if (len1 > f->count)
-        len1 = f->count;
-    len = len1;
-    while (len > 0) {
-        l = f->buf_size - f->rptr;
-        if (l > len)
-            l = len;
-        memcpy(buf, f->buf + f->rptr, l);
-        f->rptr += l;
-        if (f->rptr >= f->buf_size)
-            f->rptr = 0;
-        buf += l;
-        len -= l;
-    }
-    f->count -= len1;
-    return len1;
-}
-
 typedef enum {
     GRAPHIC_CONSOLE,
     TEXT_CONSOLE,
@@ -165,8 +115,7 @@ struct QemuConsole {
 
     Chardev *chr;
     /* fifo for key pressed */
-    QEMUFIFO out_fifo;
-    uint8_t out_fifo_buf[16];
+    Fifo8 out_fifo;
     QEMUTimer *kbd_timer;
     CoQueue dump_queue;
 
@@ -1160,21 +1109,25 @@ static int vc_chr_write(Chardev *chr, const uint8_t *buf, int len)
 static void kbd_send_chars(void *opaque)
 {
     QemuConsole *s = opaque;
-    int len;
-    uint8_t buf[16];
+    uint32_t len, avail;
 
     len = qemu_chr_be_can_write(s->chr);
-    if (len > s->out_fifo.count)
-        len = s->out_fifo.count;
-    if (len > 0) {
-        if (len > sizeof(buf))
-            len = sizeof(buf);
-        qemu_fifo_read(&s->out_fifo, buf, len);
-        qemu_chr_be_write(s->chr, buf, len);
+    avail = fifo8_num_used(&s->out_fifo);
+    if (len > avail) {
+        len = avail;
+    }
+    while (len > 0) {
+        const uint8_t *buf;
+        uint32_t size;
+
+        buf = fifo8_pop_buf(&s->out_fifo, len, &size);
+        qemu_chr_be_write(s->chr, (uint8_t *)buf, size);
+        len -= size;
+        avail -= size;
     }
     /* characters are pending: we send them a bit later (XXX:
        horrible, should change char device API) */
-    if (s->out_fifo.count > 0) {
+    if (avail > 0) {
         timer_mod(s->kbd_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 1);
     }
 }
@@ -1185,6 +1138,7 @@ void kbd_put_keysym_console(QemuConsole *s, int keysym)
     uint8_t buf[16], *q;
     CharBackend *be;
     int c;
+    uint32_t free;
 
     if (!s || (s->console_type == GRAPHIC_CONSOLE))
         return;
@@ -1228,7 +1182,8 @@ void kbd_put_keysym_console(QemuConsole *s, int keysym)
         }
         be = s->chr->be;
         if (be && be->chr_read) {
-            qemu_fifo_write(&s->out_fifo, buf, q - buf);
+            free = fifo8_num_free(&s->out_fifo);
+            fifo8_push_all(&s->out_fifo, buf, MIN(free, q - buf));
             kbd_send_chars(s);
         }
         break;
@@ -2233,8 +2188,7 @@ static void text_console_do_init(Chardev *chr, DisplayState *ds)
     int g_width = 80 * FONT_WIDTH;
     int g_height = 24 * FONT_HEIGHT;
 
-    s->out_fifo.buf = s->out_fifo_buf;
-    s->out_fifo.buf_size = sizeof(s->out_fifo_buf);
+    fifo8_create(&s->out_fifo, 16);
     s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME, kbd_send_chars, s);
     s->ds = ds;
 
-- 
2.31.1

[PATCH 2/3] ui/console: replace kbd_timer with chr_accept_input callback

[Volker Rümelin]

There's a ChardevClass chr_accept_input() callback function that
can replace the write retry timer.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 ui/console.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index e6ce29024c..7b276bfc6c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -116,7 +116,6 @@ struct QemuConsole {
     Chardev *chr;
     /* fifo for key pressed */
     Fifo8 out_fifo;
-    QEMUTimer *kbd_timer;
     CoQueue dump_queue;
 
     QTAILQ_ENTRY(QemuConsole) next;
@@ -1106,30 +1105,21 @@ static int vc_chr_write(Chardev *chr, const uint8_t *buf, int len)
     return len;
 }
 
-static void kbd_send_chars(void *opaque)
+static void kbd_send_chars(QemuConsole *s)
 {
-    QemuConsole *s = opaque;
     uint32_t len, avail;
 
     len = qemu_chr_be_can_write(s->chr);
     avail = fifo8_num_used(&s->out_fifo);
-    if (len > avail) {
-        len = avail;
-    }
-    while (len > 0) {
+    while (len > 0 && avail > 0) {
         const uint8_t *buf;
         uint32_t size;
 
-        buf = fifo8_pop_buf(&s->out_fifo, len, &size);
+        buf = fifo8_pop_buf(&s->out_fifo, MIN(len, avail), &size);
         qemu_chr_be_write(s->chr, (uint8_t *)buf, size);
-        len -= size;
+        len = qemu_chr_be_can_write(s->chr);
         avail -= size;
     }
-    /* characters are pending: we send them a bit later (XXX:
-       horrible, should change char device API) */
-    if (avail > 0) {
-        timer_mod(s->kbd_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 1);
-    }
 }
 
 /* called when an ascii key is pressed */
@@ -2141,6 +2131,14 @@ int qemu_console_get_height(QemuConsole *con, int fallback)
     return con ? surface_height(con->surface) : fallback;
 }
 
+static void vc_chr_accept_input(Chardev *chr)
+{
+    VCChardev *drv = VC_CHARDEV(chr);
+    QemuConsole *s = drv->console;
+
+    kbd_send_chars(s);
+}
+
 static void vc_chr_set_echo(Chardev *chr, bool echo)
 {
     VCChardev *drv = VC_CHARDEV(chr);
@@ -2189,7 +2187,6 @@ static void text_console_do_init(Chardev *chr, DisplayState *ds)
     int g_height = 24 * FONT_HEIGHT;
 
     fifo8_create(&s->out_fifo, 16);
-    s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME, kbd_send_chars, s);
     s->ds = ds;
 
     s->y_displayed = 0;
@@ -2439,6 +2436,7 @@ static void char_vc_class_init(ObjectClass *oc, void *data)
     cc->parse = qemu_chr_parse_vc;
     cc->open = vc_chr_open;
     cc->chr_write = vc_chr_write;
+    cc->chr_accept_input = vc_chr_accept_input;
     cc->chr_set_echo = vc_chr_set_echo;
 }
 
-- 
2.31.1

[PATCH 3/3] ui/console: remove chardev frontend connected test

[Volker Rümelin]

The test if the chardev frontend is connected in
kbd_put_keysym_console() is redundant, because the call
to qemu_chr_be_can_write() in kbd_send_chars() tests
the connected condition again.

Remove the redundant test whether the chardev frontend
is connected.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 ui/console.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/ui/console.c b/ui/console.c
index 7b276bfc6c..a06442ed86 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -28,10 +28,11 @@
 #include "qapi/error.h"
 #include "qapi/qapi-commands-ui.h"
 #include "qemu/fifo8.h"
+#include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/timer.h"
-#include "chardev/char-fe.h"
+#include "chardev/char.h"
 #include "trace.h"
 #include "exec/memory.h"
 #include "io/channel-file.h"
@@ -1126,7 +1127,6 @@ static void kbd_send_chars(QemuConsole *s)
 void kbd_put_keysym_console(QemuConsole *s, int keysym)
 {
     uint8_t buf[16], *q;
-    CharBackend *be;
     int c;
     uint32_t free;
 
@@ -1170,12 +1170,9 @@ void kbd_put_keysym_console(QemuConsole *s, int keysym)
         if (s->echo) {
             vc_chr_write(s->chr, buf, q - buf);
         }
-        be = s->chr->be;
-        if (be && be->chr_read) {
-            free = fifo8_num_free(&s->out_fifo);
-            fifo8_push_all(&s->out_fifo, buf, MIN(free, q - buf));
-            kbd_send_chars(s);
-        }
+        free = fifo8_num_free(&s->out_fifo);
+        fifo8_push_all(&s->out_fifo, buf, MIN(free, q - buf));
+        kbd_send_chars(s);
         break;
     }
 }
-- 
2.31.1

Re: [PATCH 1/3] ui/console: replace QEMUFIFO with Fifo8

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 4927 bytes --]

Hi

On Sun, Sep 12, 2021 at 4:53 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> One of the two FIFO implementations QEMUFIFO and Fifo8 is
> redundant. Replace QEMUFIFO with Fifo8.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  ui/console.c | 86 ++++++++++++----------------------------------------
>  1 file changed, 20 insertions(+), 66 deletions(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index eabbbc951c..e6ce29024c 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -27,6 +27,7 @@
>  #include "hw/qdev-core.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-ui.h"
> +#include "qemu/fifo8.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
> @@ -62,57 +63,6 @@ enum TTYState {
>      TTY_STATE_CSI,
>  };
>
> -typedef struct QEMUFIFO {
> -    uint8_t *buf;
> -    int buf_size;
> -    int count, wptr, rptr;
> -} QEMUFIFO;
> -
> -static int qemu_fifo_write(QEMUFIFO *f, const uint8_t *buf, int len1)
> -{
> -    int l, len;
> -
> -    l = f->buf_size - f->count;
> -    if (len1 > l)
> -        len1 = l;
> -    len = len1;
> -    while (len > 0) {
> -        l = f->buf_size - f->wptr;
> -        if (l > len)
> -            l = len;
> -        memcpy(f->buf + f->wptr, buf, l);
> -        f->wptr += l;
> -        if (f->wptr >= f->buf_size)
> -            f->wptr = 0;
> -        buf += l;
> -        len -= l;
> -    }
> -    f->count += len1;
> -    return len1;
> -}
> -
> -static int qemu_fifo_read(QEMUFIFO *f, uint8_t *buf, int len1)
> -{
> -    int l, len;
> -
> -    if (len1 > f->count)
> -        len1 = f->count;
> -    len = len1;
> -    while (len > 0) {
> -        l = f->buf_size - f->rptr;
> -        if (l > len)
> -            l = len;
> -        memcpy(buf, f->buf + f->rptr, l);
> -        f->rptr += l;
> -        if (f->rptr >= f->buf_size)
> -            f->rptr = 0;
> -        buf += l;
> -        len -= l;
> -    }
> -    f->count -= len1;
> -    return len1;
> -}
> -
>  typedef enum {
>      GRAPHIC_CONSOLE,
>      TEXT_CONSOLE,
> @@ -165,8 +115,7 @@ struct QemuConsole {
>
>      Chardev *chr;
>      /* fifo for key pressed */
> -    QEMUFIFO out_fifo;
> -    uint8_t out_fifo_buf[16];
> +    Fifo8 out_fifo;
>      QEMUTimer *kbd_timer;
>      CoQueue dump_queue;
>
> @@ -1160,21 +1109,25 @@ static int vc_chr_write(Chardev *chr, const
> uint8_t *buf, int len)
>  static void kbd_send_chars(void *opaque)
>  {
>      QemuConsole *s = opaque;
> -    int len;
> -    uint8_t buf[16];
> +    uint32_t len, avail;
>
>      len = qemu_chr_be_can_write(s->chr);
> -    if (len > s->out_fifo.count)
> -        len = s->out_fifo.count;
> -    if (len > 0) {
> -        if (len > sizeof(buf))
> -            len = sizeof(buf);
> -        qemu_fifo_read(&s->out_fifo, buf, len);
> -        qemu_chr_be_write(s->chr, buf, len);
> +    avail = fifo8_num_used(&s->out_fifo);
> +    if (len > avail) {
> +        len = avail;
> +    }
> +    while (len > 0) {
> +        const uint8_t *buf;
> +        uint32_t size;
> +
> +        buf = fifo8_pop_buf(&s->out_fifo, len, &size);
> +        qemu_chr_be_write(s->chr, (uint8_t *)buf, size);
> +        len -= size;
> +        avail -= size;
>      }
>      /* characters are pending: we send them a bit later (XXX:
>         horrible, should change char device API) */
> -    if (s->out_fifo.count > 0) {
> +    if (avail > 0) {
>          timer_mod(s->kbd_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> 1);
>      }
>  }
> @@ -1185,6 +1138,7 @@ void kbd_put_keysym_console(QemuConsole *s, int
> keysym)
>      uint8_t buf[16], *q;
>      CharBackend *be;
>      int c;
> +    uint32_t free;
>

Better call it num_free, to avoid symbol clash (even if we don't use free()
directly), it helps reading and can prevent mistakes.


>      if (!s || (s->console_type == GRAPHIC_CONSOLE))
>          return;
> @@ -1228,7 +1182,8 @@ void kbd_put_keysym_console(QemuConsole *s, int
> keysym)
>          }
>          be = s->chr->be;
>          if (be && be->chr_read) {
> -            qemu_fifo_write(&s->out_fifo, buf, q - buf);
> +            free = fifo8_num_free(&s->out_fifo);
> +            fifo8_push_all(&s->out_fifo, buf, MIN(free, q - buf));
>              kbd_send_chars(s);
>          }
>          break;
> @@ -2233,8 +2188,7 @@ static void text_console_do_init(Chardev *chr,
> DisplayState *ds)
>      int g_width = 80 * FONT_WIDTH;
>      int g_height = 24 * FONT_HEIGHT;
>
> -    s->out_fifo.buf = s->out_fifo_buf;
> -    s->out_fifo.buf_size = sizeof(s->out_fifo_buf);
> +    fifo8_create(&s->out_fifo, 16);
>

Missing a fif8_destroy() somewhere

     s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME, kbd_send_chars, s);
>      s->ds = ds;
>
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6632 bytes --]

Re: [PATCH 2/3] ui/console: replace kbd_timer with chr_accept_input callback

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3051 bytes --]

On Sun, Sep 12, 2021 at 5:03 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> There's a ChardevClass chr_accept_input() callback function that
> can replace the write retry timer.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  ui/console.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index e6ce29024c..7b276bfc6c 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -116,7 +116,6 @@ struct QemuConsole {
>      Chardev *chr;
>      /* fifo for key pressed */
>      Fifo8 out_fifo;
> -    QEMUTimer *kbd_timer;
>      CoQueue dump_queue;
>
>      QTAILQ_ENTRY(QemuConsole) next;
> @@ -1106,30 +1105,21 @@ static int vc_chr_write(Chardev *chr, const
> uint8_t *buf, int len)
>      return len;
>  }
>
> -static void kbd_send_chars(void *opaque)
> +static void kbd_send_chars(QemuConsole *s)
>  {
> -    QemuConsole *s = opaque;
>      uint32_t len, avail;
>
>      len = qemu_chr_be_can_write(s->chr);
>      avail = fifo8_num_used(&s->out_fifo);
> -    if (len > avail) {
> -        len = avail;
> -    }
> -    while (len > 0) {
> +    while (len > 0 && avail > 0) {
>          const uint8_t *buf;
>          uint32_t size;
>
> -        buf = fifo8_pop_buf(&s->out_fifo, len, &size);
> +        buf = fifo8_pop_buf(&s->out_fifo, MIN(len, avail), &size);
>          qemu_chr_be_write(s->chr, (uint8_t *)buf, size);
> -        len -= size;
> +        len = qemu_chr_be_can_write(s->chr);
>          avail -= size;
>      }
> -    /* characters are pending: we send them a bit later (XXX:
> -       horrible, should change char device API) */
> -    if (avail > 0) {
> -        timer_mod(s->kbd_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) +
> 1);
> -    }
>  }
>
>  /* called when an ascii key is pressed */
> @@ -2141,6 +2131,14 @@ int qemu_console_get_height(QemuConsole *con, int
> fallback)
>      return con ? surface_height(con->surface) : fallback;
>  }
>
> +static void vc_chr_accept_input(Chardev *chr)
> +{
> +    VCChardev *drv = VC_CHARDEV(chr);
> +    QemuConsole *s = drv->console;
> +
> +    kbd_send_chars(s);
> +}
> +
>  static void vc_chr_set_echo(Chardev *chr, bool echo)
>  {
>      VCChardev *drv = VC_CHARDEV(chr);
> @@ -2189,7 +2187,6 @@ static void text_console_do_init(Chardev *chr,
> DisplayState *ds)
>      int g_height = 24 * FONT_HEIGHT;
>
>      fifo8_create(&s->out_fifo, 16);
> -    s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME, kbd_send_chars, s);
>      s->ds = ds;
>
>      s->y_displayed = 0;
> @@ -2439,6 +2436,7 @@ static void char_vc_class_init(ObjectClass *oc, void
> *data)
>      cc->parse = qemu_chr_parse_vc;
>      cc->open = vc_chr_open;
>      cc->chr_write = vc_chr_write;
> +    cc->chr_accept_input = vc_chr_accept_input;
>      cc->chr_set_echo = vc_chr_set_echo;
>  }
>
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4136 bytes --]

Re: [PATCH 3/3] ui/console: remove chardev frontend connected test

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]

On Sun, Sep 12, 2021 at 4:53 PM Volker Rümelin <vr_qemu@t-online.de> wrote:

> The test if the chardev frontend is connected in
> kbd_put_keysym_console() is redundant, because the call
> to qemu_chr_be_can_write() in kbd_send_chars() tests
> the connected condition again.
>
> Remove the redundant test whether the chardev frontend
> is connected.
>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  ui/console.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/ui/console.c b/ui/console.c
> index 7b276bfc6c..a06442ed86 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -28,10 +28,11 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-ui.h"
>  #include "qemu/fifo8.h"
> +#include "qemu/main-loop.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
> -#include "chardev/char-fe.h"
> +#include "chardev/char.h"
>  #include "trace.h"
>  #include "exec/memory.h"
>  #include "io/channel-file.h"
> @@ -1126,7 +1127,6 @@ static void kbd_send_chars(QemuConsole *s)
>  void kbd_put_keysym_console(QemuConsole *s, int keysym)
>  {
>      uint8_t buf[16], *q;
> -    CharBackend *be;
>      int c;
>      uint32_t free;
>
> @@ -1170,12 +1170,9 @@ void kbd_put_keysym_console(QemuConsole *s, int
> keysym)
>          if (s->echo) {
>              vc_chr_write(s->chr, buf, q - buf);
>          }
> -        be = s->chr->be;
> -        if (be && be->chr_read) {
> -            free = fifo8_num_free(&s->out_fifo);
> -            fifo8_push_all(&s->out_fifo, buf, MIN(free, q - buf));
> -            kbd_send_chars(s);
> -        }
> +        free = fifo8_num_free(&s->out_fifo);
> +        fifo8_push_all(&s->out_fifo, buf, MIN(free, q - buf));
> +        kbd_send_chars(s);
>          break;
>      }
>  }
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3014 bytes --]

Re: [PATCH 1/3] ui/console: replace QEMUFIFO with Fifo8

[Volker Rümelin]

>
>     @@ -1185,6 +1138,7 @@ void kbd_put_keysym_console(QemuConsole *s,
>     int keysym)
>          uint8_t buf[16], *q;
>          CharBackend *be;
>          int c;
>     +    uint32_t free;
>
>
> Better call it num_free, to avoid symbol clash (even if we don't use 
> free() directly), it helps reading and can prevent mistakes.
>

Hi,

OK, I'll send a version 2 patch.

>
>          if (!s || (s->console_type == GRAPHIC_CONSOLE))
>              return;
>

>     @@ -2233,8 +2188,7 @@ static void text_console_do_init(Chardev
>     *chr, DisplayState *ds)
>          int g_width = 80 * FONT_WIDTH;
>          int g_height = 24 * FONT_HEIGHT;
>
>     -    s->out_fifo.buf = s->out_fifo_buf;
>     -    s->out_fifo.buf_size = sizeof(s->out_fifo_buf);
>     +    fifo8_create(&s->out_fifo, 16);
>
>
> Missing a fif8_destroy() somewhere
>

An opened text console stays open until QEMU exits. There's no 
text_console_close() function. Just like there's a ChardevClass open 
call but no close call. I think this is one of the many cases in QEMU 
where resources get allocated for the lifetime of QEMU.

With best regards,
Volker

>          s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>     kbd_send_chars, s);
>          s->ds = ds;
>

Re: [PATCH 1/3] ui/console: replace QEMUFIFO with Fifo8

[Volker Rümelin]

Am 12.09.21 um 19:58 schrieb Volker Rümelin:
>>
>>     @@ -1185,6 +1138,7 @@ void kbd_put_keysym_console(QemuConsole *s,
>>     int keysym)
>>          uint8_t buf[16], *q;
>>          CharBackend *be;
>>          int c;
>>     +    uint32_t free;
>>
>>
>> Better call it num_free, to avoid symbol clash (even if we don't use 
>> free() directly), it helps reading and can prevent mistakes.
>>
>
> Hi,
>
> OK, I'll send a version 2 patch.
>
>>
>>          if (!s || (s->console_type == GRAPHIC_CONSOLE))
>>              return;
>>
>
>>     @@ -2233,8 +2188,7 @@ static void text_console_do_init(Chardev
>>     *chr, DisplayState *ds)
>>          int g_width = 80 * FONT_WIDTH;
>>          int g_height = 24 * FONT_HEIGHT;
>>
>>     -    s->out_fifo.buf = s->out_fifo_buf;
>>     -    s->out_fifo.buf_size = sizeof(s->out_fifo_buf);
>>     +    fifo8_create(&s->out_fifo, 16);
>>
>>
>> Missing a fif8_destroy() somewhere
>>
>
> An opened text console stays open until QEMU exits. There's no 
> text_console_close() function. Just like there's a ChardevClass open 
> call but no close call. I think this is one of the many cases in QEMU 
> where resources get allocated for the lifetime of QEMU.

Sorry, I think my last four sentences are simply wrong. Please ignore this.

>
>
> With best regards,
> Volker
>
>>          s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>>     kbd_send_chars, s);
>>          s->ds = ds;
>>
>

Re: [PATCH 1/3] ui/console: replace QEMUFIFO with Fifo8

[Volker Rümelin]

>
>
>     @@ -2233,8 +2188,7 @@ static void text_console_do_init(Chardev
>     *chr, DisplayState *ds)
>          int g_width = 80 * FONT_WIDTH;
>          int g_height = 24 * FONT_HEIGHT;
>
>     -    s->out_fifo.buf = s->out_fifo_buf;
>     -    s->out_fifo.buf_size = sizeof(s->out_fifo_buf);
>     +    fifo8_create(&s->out_fifo, 16);
>
>
> Missing a fif8_destroy() somewhere

Hi,

there's no function to close a text console. An opened text console 
remains open until QEMU exits. Currently QEMU doesn't free allocated 
text console resources.

With best regards,
Volker

>
>          s->kbd_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
>     kbd_send_chars, s);
>          s->ds = ds;
>
>
>