[PATCH v2 0/8] chardev/mux: implement frontend detach

[PATCH v2 0/8] chardev/mux: implement frontend detach

[Roman Penyaev]

Frontend device can be detached in run-time, which can lead to a
"Chardev 'MUX' is busy" error (see the last patch with the test case
implementation). This series implements frontend detach for the
multiplexer based on bitset, which provides the ability to attach or
detach frontend devices in any order.

Also first patches do some refactoring the purpose of which is to make
integer unsigned where possible (such as sizes or lengths).

v1->v2:
  * improve if condition and assert for bit verification

Roman Penyaev (8):
  chardev/char: fix qemu_chr_is_busy() check
  chardev/chardev-internal: remove unused `max_size` struct member
  chardev/mux: use bool type for `linestart` and `term_got_escape`
  chardev/mux: convert size members to unsigned int
  chardev/mux: introduce `mux_chr_attach_frontend() call
  chardev/mux: switch mux frontends management to bitset
  chardev/mux: implement detach of frontends from mux
  tests/unit/test-char: implement a few mux remove test cases

 chardev/char-fe.c          | 13 ++----
 chardev/char-mux.c         | 90 ++++++++++++++++++++++++++++----------
 chardev/char.c             |  2 +-
 chardev/chardev-internal.h | 16 ++++---
 include/chardev/char-fe.h  |  2 +-
 tests/unit/test-char.c     | 24 +++++++++-
 6 files changed, 105 insertions(+), 42 deletions(-)

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org


-- 
2.34.1

[PATCH v2 1/8] chardev/char: fix qemu_chr_is_busy() check

[Roman Penyaev]

`mux_cnt` struct member never goes negative or decrements,
so mux chardev can be !busy only when there are no
frontends attached. This patch fixes the always-true
check.

Fixes: a4afa548fc6d ("char: move front end handlers in CharBackend")
Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index c0cc52824b48..f54dc3a86286 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
 {
     if (CHARDEV_IS_MUX(s)) {
         MuxChardev *d = MUX_CHARDEV(s);
-        return d->mux_cnt >= 0;
+        return d->mux_cnt > 0;
     } else {
         return s->be != NULL;
     }
-- 
2.34.1

[PATCH v2 2/8] chardev/chardev-internal: remove unused `max_size` struct member

[Roman Penyaev]

Clean up forgotten leftovers.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/chardev-internal.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index 4e03af31476c..c3024b51fdda 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -40,7 +40,6 @@ struct MuxChardev {
     int focus;
     int mux_cnt;
     int term_got_escape;
-    int max_size;
     /* Intermediate input buffer catches escape sequences even if the
        currently active device is not accepting any input - but only until it
        is full as well. */
-- 
2.34.1

[PATCH v2 3/8] chardev/mux: use bool type for `linestart` and `term_got_escape`

[Roman Penyaev]

Those are boolean variables, not signed integers.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-mux.c         | 10 +++++-----
 chardev/chardev-internal.h |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index ee2d47b20d9b..728596c6f346 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -73,11 +73,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
                  * qemu_chr_fe_write and background I/O callbacks */
                 qemu_chr_fe_write_all(&d->chr,
                                       (uint8_t *)buf1, strlen(buf1));
-                d->linestart = 0;
+                d->linestart = false;
             }
             ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
             if (buf[i] == '\n') {
-                d->linestart = 1;
+                d->linestart = true;
             }
         }
     }
@@ -145,7 +145,7 @@ static void mux_chr_be_event(Chardev *chr, QEMUChrEvent event)
 static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
 {
     if (d->term_got_escape) {
-        d->term_got_escape = 0;
+        d->term_got_escape = false;
         if (ch == term_escape_char) {
             goto send_char;
         }
@@ -175,11 +175,11 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
         case 't':
             d->timestamps = !d->timestamps;
             d->timestamps_start = -1;
-            d->linestart = 0;
+            d->linestart = false;
             break;
         }
     } else if (ch == term_escape_char) {
-        d->term_got_escape = 1;
+        d->term_got_escape = true;
     } else {
     send_char:
         return 1;
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index c3024b51fdda..975c16de803e 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -39,7 +39,7 @@ struct MuxChardev {
     CharBackend chr;
     int focus;
     int mux_cnt;
-    int term_got_escape;
+    bool term_got_escape;
     /* Intermediate input buffer catches escape sequences even if the
        currently active device is not accepting any input - but only until it
        is full as well. */
@@ -49,7 +49,7 @@ struct MuxChardev {
     int timestamps;
 
     /* Protected by the Chardev chr_write_lock.  */
-    int linestart;
+    bool linestart;
     int64_t timestamps_start;
 };
 typedef struct MuxChardev MuxChardev;
-- 
2.34.1

[PATCH v2 4/8] chardev/mux: convert size members to unsigned int

[Roman Penyaev]

There is no sense to keep `focus`, `mux_cnt`, `prod`, `cons`
and `tag` variables as signed, those represent either size,
either position in array, which both are unsigned.

`focus` member of `MuxChardev` is kept signed, because initially
set to -1.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-fe.c          |  2 +-
 chardev/char-mux.c         | 10 +++++-----
 chardev/chardev-internal.h |  8 ++++----
 include/chardev/char-fe.h  |  2 +-
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index b214ba3802b1..69b47d16bdfa 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -191,7 +191,7 @@ bool qemu_chr_fe_backend_open(CharBackend *be)
 
 bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
 {
-    int tag = 0;
+    unsigned int tag = 0;
 
     if (s) {
         if (CHARDEV_IS_MUX(s)) {
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 728596c6f346..b2d7abf2fc01 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -124,7 +124,8 @@ static void mux_print_help(Chardev *chr)
     }
 }
 
-static void mux_chr_send_event(MuxChardev *d, int mux_nr, QEMUChrEvent event)
+static void mux_chr_send_event(MuxChardev *d, unsigned int mux_nr,
+                               QEMUChrEvent event)
 {
     CharBackend *be = d->backends[mux_nr];
 
@@ -242,7 +243,7 @@ static void mux_chr_read(void *opaque, const uint8_t *buf, int size)
 void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
-    int i;
+    unsigned int i;
 
     if (!muxes_opened) {
         return;
@@ -275,7 +276,7 @@ static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
 static void char_mux_finalize(Object *obj)
 {
     MuxChardev *d = MUX_CHARDEV(obj);
-    int i;
+    unsigned int i;
 
     for (i = 0; i < d->mux_cnt; i++) {
         CharBackend *be = d->backends[i];
@@ -300,11 +301,10 @@ static void mux_chr_update_read_handlers(Chardev *chr)
                                   chr->gcontext, true, false);
 }
 
-void mux_set_focus(Chardev *chr, int focus)
+void mux_set_focus(Chardev *chr, unsigned int focus)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
 
-    assert(focus >= 0);
     assert(focus < d->mux_cnt);
 
     if (d->focus != -1) {
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index 975c16de803e..ab93f6ea1720 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -38,14 +38,14 @@ struct MuxChardev {
     CharBackend *backends[MAX_MUX];
     CharBackend chr;
     int focus;
-    int mux_cnt;
+    unsigned int mux_cnt;
     bool term_got_escape;
     /* Intermediate input buffer catches escape sequences even if the
        currently active device is not accepting any input - but only until it
        is full as well. */
     unsigned char buffer[MAX_MUX][MUX_BUFFER_SIZE];
-    int prod[MAX_MUX];
-    int cons[MAX_MUX];
+    unsigned int prod[MAX_MUX];
+    unsigned int cons[MAX_MUX];
     int timestamps;
 
     /* Protected by the Chardev chr_write_lock.  */
@@ -59,7 +59,7 @@ DECLARE_INSTANCE_CHECKER(MuxChardev, MUX_CHARDEV,
 #define CHARDEV_IS_MUX(chr)                             \
     object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
 
-void mux_set_focus(Chardev *chr, int focus);
+void mux_set_focus(Chardev *chr, unsigned int focus);
 void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event);
 
 Object *get_chardevs_root(void);
diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index 3310449eaf03..8ef05b3dd095 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -20,7 +20,7 @@ struct CharBackend {
     IOReadHandler *chr_read;
     BackendChangeHandler *chr_be_change;
     void *opaque;
-    int tag;
+    unsigned int tag;
     bool fe_is_open;
 };
 
-- 
2.34.1

[PATCH v2 5/8] chardev/mux: introduce `mux_chr_attach_frontend() call

[Roman Penyaev]

Move away logic which attaches frontend device to a mux
from `char-fe.c` to actual `char-mux.c` implementation
and make it a separate function.

No logic changes are made.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-fe.c          |  9 +--------
 chardev/char-mux.c         | 17 +++++++++++++++++
 chardev/chardev-internal.h |  2 ++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 69b47d16bdfa..3b8771ca2ac4 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -197,16 +197,9 @@ bool qemu_chr_fe_init(CharBackend *b, Chardev *s, Error **errp)
         if (CHARDEV_IS_MUX(s)) {
             MuxChardev *d = MUX_CHARDEV(s);
 
-            if (d->mux_cnt >= MAX_MUX) {
-                error_setg(errp,
-                           "too many uses of multiplexed chardev '%s'"
-                           " (maximum is " stringify(MAX_MUX) ")",
-                           s->label);
+            if (!mux_chr_attach_frontend(d, b, &tag, errp)) {
                 return false;
             }
-
-            d->backends[d->mux_cnt] = b;
-            tag = d->mux_cnt++;
         } else if (s->be) {
             error_setg(errp, "chardev '%s' is already in use", s->label);
             return false;
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index b2d7abf2fc01..9294f955462e 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -301,6 +301,23 @@ static void mux_chr_update_read_handlers(Chardev *chr)
                                   chr->gcontext, true, false);
 }
 
+bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
+                             unsigned int *tag, Error **errp)
+{
+    if (d->mux_cnt >= MAX_MUX) {
+        error_setg(errp,
+                   "too many uses of multiplexed chardev '%s'"
+                   " (maximum is " stringify(MAX_MUX) ")",
+                   d->parent.label);
+        return false;
+    }
+
+    d->backends[d->mux_cnt] = b;
+    *tag = d->mux_cnt++;
+
+    return true;
+}
+
 void mux_set_focus(Chardev *chr, unsigned int focus)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index ab93f6ea1720..8126ce180690 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -59,6 +59,8 @@ DECLARE_INSTANCE_CHECKER(MuxChardev, MUX_CHARDEV,
 #define CHARDEV_IS_MUX(chr)                             \
     object_dynamic_cast(OBJECT(chr), TYPE_CHARDEV_MUX)
 
+bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
+                             unsigned int *tag, Error **errp);
 void mux_set_focus(Chardev *chr, unsigned int focus);
 void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event);
 
-- 
2.34.1

[PATCH v2 6/8] chardev/mux: switch mux frontends management to bitset

[Roman Penyaev]

Frontends can be attached and detached during run-time (although detach
is not implemented, but will follow). Counter variable of muxes is not
enough for proper attach/detach management, so this patch implements
bitset: if bit is set for the `mux_bitset` variable, then frontend
device can be found in the `backend` array (yes, huge confusion with
backend and frontends names).

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-mux.c         | 42 +++++++++++++++++++++++++-------------
 chardev/char.c             |  2 +-
 chardev/chardev-internal.h |  2 +-
 3 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 9294f955462e..4fc619b2da70 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/bitops.h"
 #include "chardev/char.h"
 #include "sysemu/block-backend.h"
 #include "qapi/qapi-commands-control.h"
@@ -168,12 +169,19 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
         case 'b':
             qemu_chr_be_event(chr, CHR_EVENT_BREAK);
             break;
-        case 'c':
-            assert(d->mux_cnt > 0); /* handler registered with first fe */
+        case 'c': {
+            unsigned int bit;
+
+            /* Handler registered with first fe */
+            assert(d->mux_bitset != 0);
             /* Switch to the next registered device */
-            mux_set_focus(chr, (d->focus + 1) % d->mux_cnt);
+            bit = find_next_bit(&d->mux_bitset, MAX_MUX, d->focus + 1);
+            if (bit >= MAX_MUX) {
+                bit = find_next_bit(&d->mux_bitset, MAX_MUX, 0);
+            }
+            mux_set_focus(chr, bit);
             break;
-        case 't':
+        } case 't':
             d->timestamps = !d->timestamps;
             d->timestamps_start = -1;
             d->linestart = false;
@@ -243,15 +251,16 @@ static void mux_chr_read(void *opaque, const uint8_t *buf, int size)
 void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
-    unsigned int i;
+    int bit;
 
     if (!muxes_opened) {
         return;
     }
 
     /* Send the event to all registered listeners */
-    for (i = 0; i < d->mux_cnt; i++) {
-        mux_chr_send_event(d, i, event);
+    bit = -1;
+    while ((bit = find_next_bit(&d->mux_bitset, MAX_MUX, bit + 1)) < MAX_MUX) {
+        mux_chr_send_event(d, bit, event);
     }
 }
 
@@ -276,10 +285,11 @@ static GSource *mux_chr_add_watch(Chardev *s, GIOCondition cond)
 static void char_mux_finalize(Object *obj)
 {
     MuxChardev *d = MUX_CHARDEV(obj);
-    unsigned int i;
+    int bit;
 
-    for (i = 0; i < d->mux_cnt; i++) {
-        CharBackend *be = d->backends[i];
+    bit = -1;
+    while ((bit = find_next_bit(&d->mux_bitset, MAX_MUX, bit + 1)) < MAX_MUX) {
+        CharBackend *be = d->backends[bit];
         if (be) {
             be->chr = NULL;
         }
@@ -304,7 +314,10 @@ static void mux_chr_update_read_handlers(Chardev *chr)
 bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
                              unsigned int *tag, Error **errp)
 {
-    if (d->mux_cnt >= MAX_MUX) {
+    unsigned int bit;
+
+    bit = find_next_zero_bit(&d->mux_bitset, MAX_MUX, 0);
+    if (bit >= MAX_MUX) {
         error_setg(errp,
                    "too many uses of multiplexed chardev '%s'"
                    " (maximum is " stringify(MAX_MUX) ")",
@@ -312,8 +325,9 @@ bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
         return false;
     }
 
-    d->backends[d->mux_cnt] = b;
-    *tag = d->mux_cnt++;
+    d->mux_bitset |= (1 << bit);
+    d->backends[bit] = b;
+    *tag = bit;
 
     return true;
 }
@@ -322,7 +336,7 @@ void mux_set_focus(Chardev *chr, unsigned int focus)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
 
-    assert(focus < d->mux_cnt);
+    assert(find_next_bit(&d->mux_bitset, MAX_MUX, focus) == focus);
 
     if (d->focus != -1) {
         mux_chr_send_event(d, d->focus, CHR_EVENT_MUX_OUT);
diff --git a/chardev/char.c b/chardev/char.c
index f54dc3a86286..a1722aa076d9 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -333,7 +333,7 @@ static bool qemu_chr_is_busy(Chardev *s)
 {
     if (CHARDEV_IS_MUX(s)) {
         MuxChardev *d = MUX_CHARDEV(s);
-        return d->mux_cnt > 0;
+        return d->mux_bitset != 0;
     } else {
         return s->be != NULL;
     }
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index 8126ce180690..b89aada5413b 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -37,8 +37,8 @@ struct MuxChardev {
     Chardev parent;
     CharBackend *backends[MAX_MUX];
     CharBackend chr;
+    unsigned long mux_bitset;
     int focus;
-    unsigned int mux_cnt;
     bool term_got_escape;
     /* Intermediate input buffer catches escape sequences even if the
        currently active device is not accepting any input - but only until it
-- 
2.34.1

[PATCH v2 7/8] chardev/mux: implement detach of frontends from mux

[Roman Penyaev]

With bitset management now it becomes feasible to implement
the logic of detaching frontends from multiplexer.

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 chardev/char-fe.c          |  2 +-
 chardev/char-mux.c         | 21 ++++++++++++++++++---
 chardev/chardev-internal.h |  1 +
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index 3b8771ca2ac4..8ac6bebb6f74 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -225,7 +225,7 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
         }
         if (CHARDEV_IS_MUX(b->chr)) {
             MuxChardev *d = MUX_CHARDEV(b->chr);
-            d->backends[b->tag] = NULL;
+            mux_chr_detach_frontend(d, b->tag);
         }
         if (del) {
             Object *obj = OBJECT(b->chr);
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 4fc619b2da70..bda5c45e6058 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -290,10 +290,10 @@ static void char_mux_finalize(Object *obj)
     bit = -1;
     while ((bit = find_next_bit(&d->mux_bitset, MAX_MUX, bit + 1)) < MAX_MUX) {
         CharBackend *be = d->backends[bit];
-        if (be) {
-            be->chr = NULL;
-        }
+        be->chr = NULL;
+        d->backends[bit] = NULL;
     }
+    d->mux_bitset = 0;
     qemu_chr_fe_deinit(&d->chr, false);
 }
 
@@ -332,6 +332,21 @@ bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
     return true;
 }
 
+bool mux_chr_detach_frontend(MuxChardev *d, unsigned int tag)
+{
+    unsigned int bit;
+
+    bit = find_next_bit(&d->mux_bitset, MAX_MUX, tag);
+    if (bit != tag) {
+        return false;
+    }
+
+    d->mux_bitset &= ~(1 << bit);
+    d->backends[bit] = NULL;
+
+    return true;
+}
+
 void mux_set_focus(Chardev *chr, unsigned int focus)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
index b89aada5413b..853807f3cb88 100644
--- a/chardev/chardev-internal.h
+++ b/chardev/chardev-internal.h
@@ -61,6 +61,7 @@ DECLARE_INSTANCE_CHECKER(MuxChardev, MUX_CHARDEV,
 
 bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
                              unsigned int *tag, Error **errp);
+bool mux_chr_detach_frontend(MuxChardev *d, unsigned int tag);
 void mux_set_focus(Chardev *chr, unsigned int focus);
 void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event);
 
-- 
2.34.1

[PATCH v2 8/8] tests/unit/test-char: implement a few mux remove test cases

[Roman Penyaev]

This patch tests:

1. feasibility of removing mux which does not have frontends attached
   or frontends were prior detached.
2. inability to remove mux which has frontends attached (mux is "busy")

Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: qemu-devel@nongnu.org
---
 tests/unit/test-char.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
index f273ce522612..2837dbb863a8 100644
--- a/tests/unit/test-char.c
+++ b/tests/unit/test-char.c
@@ -1,6 +1,7 @@
 #include "qemu/osdep.h"
 #include <glib/gstdio.h>
 
+#include "qapi/error.h"
 #include "qemu/config-file.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -184,6 +185,21 @@ static void char_mux_test(void)
     char *data;
     FeHandler h1 = { 0, false, 0, false, }, h2 = { 0, false, 0, false, };
     CharBackend chr_be1, chr_be2;
+    Error *error = NULL;
+
+    /* Create mux and chardev to be immediately removed */
+    opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
+                            1, &error_abort);
+    qemu_opt_set(opts, "backend", "ringbuf", &error_abort);
+    qemu_opt_set(opts, "size", "128", &error_abort);
+    qemu_opt_set(opts, "mux", "on", &error_abort);
+    chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
+    g_assert_nonnull(chr);
+    qemu_opts_del(opts);
+
+    /* Remove just created mux and chardev */
+    qmp_chardev_remove("mux-label", &error_abort);
+    qmp_chardev_remove("mux-label-base", &error_abort);
 
     opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
                             1, &error_abort);
@@ -334,7 +350,13 @@ static void char_mux_test(void)
     g_free(data);
 
     qemu_chr_fe_deinit(&chr_be1, false);
-    qemu_chr_fe_deinit(&chr_be2, true);
+
+    error = NULL;
+    qmp_chardev_remove("mux-label", &error);
+    g_assert_cmpstr(error_get_pretty(error), ==, "Chardev 'mux-label' is busy");
+
+    qemu_chr_fe_deinit(&chr_be2, false);
+    qmp_chardev_remove("mux-label", &error_abort);
 }
 
 
-- 
2.34.1

Re: [PATCH v2 8/8] tests/unit/test-char: implement a few mux remove test cases

[Marc-André Lureau]

Hi

On Mon, Oct 14, 2024 at 7:26 PM Roman Penyaev <r.peniaev@gmail.com> wrote:
>
> This patch tests:
>
> 1. feasibility of removing mux which does not have frontends attached
>    or frontends were prior detached.
> 2. inability to remove mux which has frontends attached (mux is "busy")
>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  tests/unit/test-char.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c
> index f273ce522612..2837dbb863a8 100644
> --- a/tests/unit/test-char.c
> +++ b/tests/unit/test-char.c
> @@ -1,6 +1,7 @@
>  #include "qemu/osdep.h"
>  #include <glib/gstdio.h>
>
> +#include "qapi/error.h"
>  #include "qemu/config-file.h"
>  #include "qemu/module.h"
>  #include "qemu/option.h"
> @@ -184,6 +185,21 @@ static void char_mux_test(void)
>      char *data;
>      FeHandler h1 = { 0, false, 0, false, }, h2 = { 0, false, 0, false, };
>      CharBackend chr_be1, chr_be2;
> +    Error *error = NULL;
> +
> +    /* Create mux and chardev to be immediately removed */
> +    opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
> +                            1, &error_abort);
> +    qemu_opt_set(opts, "backend", "ringbuf", &error_abort);
> +    qemu_opt_set(opts, "size", "128", &error_abort);
> +    qemu_opt_set(opts, "mux", "on", &error_abort);
> +    chr = qemu_chr_new_from_opts(opts, NULL, &error_abort);
> +    g_assert_nonnull(chr);
> +    qemu_opts_del(opts);
> +
> +    /* Remove just created mux and chardev */
> +    qmp_chardev_remove("mux-label", &error_abort);
> +    qmp_chardev_remove("mux-label-base", &error_abort);
>
>      opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
>                              1, &error_abort);
> @@ -334,7 +350,13 @@ static void char_mux_test(void)
>      g_free(data);
>
>      qemu_chr_fe_deinit(&chr_be1, false);
> -    qemu_chr_fe_deinit(&chr_be2, true);
> +
> +    error = NULL;

Unnecessary assignment,

> +    qmp_chardev_remove("mux-label", &error);
> +    g_assert_cmpstr(error_get_pretty(error), ==, "Chardev 'mux-label' is busy");

However, error_free() is missing.
I'll touch on commit

thanks

> +
> +    qemu_chr_fe_deinit(&chr_be2, false);
> +    qmp_chardev_remove("mux-label", &error_abort);
>  }
>
>
> --
> 2.34.1
>

Re: [PATCH v2 8/8] tests/unit/test-char: implement a few mux remove test cases

[Roman Penyaev]

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

Hi,

On Tue, Oct 15, 2024, 10:50 Marc-André Lureau <marcandre.lureau@redhat.com>
wrote:

[cut]

>      qemu_chr_fe_deinit(&chr_be1, false);
> > -    qemu_chr_fe_deinit(&chr_be2, true);
> > +
> > +    error = NULL;
>
> Unnecessary assignment,
>
> > +    qmp_chardev_remove("mux-label", &error);
> > +    g_assert_cmpstr(error_get_pretty(error), ==, "Chardev 'mux-label'
> is busy");
>
> However, error_free() is missing.
> I'll touch on commit
>
> thanks
>


My bad, thanks for taking care of this.

--
Roman

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

Re: [PATCH v2 7/8] chardev/mux: implement detach of frontends from mux

[CLEMENT MATHIEU--DRIF]

On 14/10/2024 17:24, Roman Penyaev wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
> 
> 
> With bitset management now it becomes feasible to implement
> the logic of detaching frontends from multiplexer.
> 
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>   chardev/char-fe.c          |  2 +-
>   chardev/char-mux.c         | 21 ++++++++++++++++++---
>   chardev/chardev-internal.h |  1 +
>   3 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index 3b8771ca2ac4..8ac6bebb6f74 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -225,7 +225,7 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
>           }
>           if (CHARDEV_IS_MUX(b->chr)) {
>               MuxChardev *d = MUX_CHARDEV(b->chr);
> -            d->backends[b->tag] = NULL;
> +            mux_chr_detach_frontend(d, b->tag);
>           }
>           if (del) {
>               Object *obj = OBJECT(b->chr);
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index 4fc619b2da70..bda5c45e6058 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -290,10 +290,10 @@ static void char_mux_finalize(Object *obj)
>       bit = -1;
>       while ((bit = find_next_bit(&d->mux_bitset, MAX_MUX, bit + 1)) < MAX_MUX) {
>           CharBackend *be = d->backends[bit];
> -        if (be) {
> -            be->chr = NULL;
> -        }
> +        be->chr = NULL;
> +        d->backends[bit] = NULL;
>       }
> +    d->mux_bitset = 0;
>       qemu_chr_fe_deinit(&d->chr, false);
>   }
> 
> @@ -332,6 +332,21 @@ bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
>       return true;
>   }
> 
> +bool mux_chr_detach_frontend(MuxChardev *d, unsigned int tag)
> +{
> +    unsigned int bit;
> +
> +    bit = find_next_bit(&d->mux_bitset, MAX_MUX, tag);
> +    if (bit != tag) {
> +        return false;
> +    }
> +
> +    d->mux_bitset &= ~(1 << bit);

mux_bitset is unsigned long, I think we should use 1ul here even id 
MAX_MUX is a low value

> +    d->backends[bit] = NULL;
> +
> +    return true;
> +}
> +
>   void mux_set_focus(Chardev *chr, unsigned int focus)
>   {
>       MuxChardev *d = MUX_CHARDEV(chr);
> diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> index b89aada5413b..853807f3cb88 100644
> --- a/chardev/chardev-internal.h
> +++ b/chardev/chardev-internal.h
> @@ -61,6 +61,7 @@ DECLARE_INSTANCE_CHECKER(MuxChardev, MUX_CHARDEV,
> 
>   bool mux_chr_attach_frontend(MuxChardev *d, CharBackend *b,
>                                unsigned int *tag, Error **errp);
> +bool mux_chr_detach_frontend(MuxChardev *d, unsigned int tag);
>   void mux_set_focus(Chardev *chr, unsigned int focus);
>   void mux_chr_send_all_event(Chardev *chr, QEMUChrEvent event);
> 
> --
> 2.34.1
> 
> 

Re: [PATCH v2 3/8] chardev/mux: use bool type for `linestart` and `term_got_escape`

[CLEMENT MATHIEU--DRIF]

Reviewed-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>


On 14/10/2024 17:24, Roman Penyaev wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Those are boolean variables, not signed integers.
>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>   chardev/char-mux.c         | 10 +++++-----
>   chardev/chardev-internal.h |  4 ++--
>   2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> index ee2d47b20d9b..728596c6f346 100644
> --- a/chardev/char-mux.c
> +++ b/chardev/char-mux.c
> @@ -73,11 +73,11 @@ static int mux_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                    * qemu_chr_fe_write and background I/O callbacks */
>                   qemu_chr_fe_write_all(&d->chr,
>                                         (uint8_t *)buf1, strlen(buf1));
> -                d->linestart = 0;
> +                d->linestart = false;
>               }
>               ret += qemu_chr_fe_write(&d->chr, buf + i, 1);
>               if (buf[i] == '\n') {
> -                d->linestart = 1;
> +                d->linestart = true;
>               }
>           }
>       }
> @@ -145,7 +145,7 @@ static void mux_chr_be_event(Chardev *chr, QEMUChrEvent event)
>   static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
>   {
>       if (d->term_got_escape) {
> -        d->term_got_escape = 0;
> +        d->term_got_escape = false;
>           if (ch == term_escape_char) {
>               goto send_char;
>           }
> @@ -175,11 +175,11 @@ static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
>           case 't':
>               d->timestamps = !d->timestamps;
>               d->timestamps_start = -1;
> -            d->linestart = 0;
> +            d->linestart = false;
>               break;
>           }
>       } else if (ch == term_escape_char) {
> -        d->term_got_escape = 1;
> +        d->term_got_escape = true;
>       } else {
>       send_char:
>           return 1;
> diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> index c3024b51fdda..975c16de803e 100644
> --- a/chardev/chardev-internal.h
> +++ b/chardev/chardev-internal.h
> @@ -39,7 +39,7 @@ struct MuxChardev {
>       CharBackend chr;
>       int focus;
>       int mux_cnt;
> -    int term_got_escape;
> +    bool term_got_escape;
>       /* Intermediate input buffer catches escape sequences even if the
>          currently active device is not accepting any input - but only until it
>          is full as well. */
> @@ -49,7 +49,7 @@ struct MuxChardev {
>       int timestamps;
>
>       /* Protected by the Chardev chr_write_lock.  */
> -    int linestart;
> +    bool linestart;
>       int64_t timestamps_start;
>   };
>   typedef struct MuxChardev MuxChardev;
> --
> 2.34.1
>
>

Re: [PATCH v2 2/8] chardev/chardev-internal: remove unused `max_size` struct member

[CLEMENT MATHIEU--DRIF]

Reviewed-by: Clément Mathieu--Drif <clement.mathieu--drif@eviden.com>


On 14/10/2024 17:24, Roman Penyaev wrote:
> Caution: External email. Do not open attachments or click links, unless this email comes from a known sender and you know the content is safe.
>
>
> Clean up forgotten leftovers.
>
> Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>   chardev/chardev-internal.h | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/chardev/chardev-internal.h b/chardev/chardev-internal.h
> index 4e03af31476c..c3024b51fdda 100644
> --- a/chardev/chardev-internal.h
> +++ b/chardev/chardev-internal.h
> @@ -40,7 +40,6 @@ struct MuxChardev {
>       int focus;
>       int mux_cnt;
>       int term_got_escape;
> -    int max_size;
>       /* Intermediate input buffer catches escape sequences even if the
>          currently active device is not accepting any input - but only until it
>          is full as well. */
> --
> 2.34.1
>
>

Re: [PATCH v2 7/8] chardev/mux: implement detach of frontends from mux

[Roman Penyaev]

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

Hi,

On Tue, Oct 22, 2024, 07:21 CLEMENT MATHIEU--DRIF <
clement.mathieu--drif@eviden.com> wrote:

>
>
> On 14/10/2024 17:24, Roman Penyaev wrote:
> > Caution: External email. Do not open attachments or click links, unless
> this email comes from a known sender and you know the content is safe.
> >
> >
> > With bitset management now it becomes feasible to implement
> > the logic of detaching frontends from multiplexer.
> >
> > Signed-off-by: Roman Penyaev <r.peniaev@gmail.com>
> > Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
> > Cc: qemu-devel@nongnu.org
> > ---
> >   chardev/char-fe.c          |  2 +-
> >   chardev/char-mux.c         | 21 ++++++++++++++++++---
> >   chardev/chardev-internal.h |  1 +
> >   3 files changed, 20 insertions(+), 4 deletions(-)
> >
> > diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> > index 3b8771ca2ac4..8ac6bebb6f74 100644
> > --- a/chardev/char-fe.c
> > +++ b/chardev/char-fe.c
> > @@ -225,7 +225,7 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
> >           }
> >           if (CHARDEV_IS_MUX(b->chr)) {
> >               MuxChardev *d = MUX_CHARDEV(b->chr);
> > -            d->backends[b->tag] = NULL;
> > +            mux_chr_detach_frontend(d, b->tag);
> >           }
> >           if (del) {
> >               Object *obj = OBJECT(b->chr);
> > diff --git a/chardev/char-mux.c b/chardev/char-mux.c
> > index 4fc619b2da70..bda5c45e6058 100644
> > --- a/chardev/char-mux.c
> > +++ b/chardev/char-mux.c
> > @@ -290,10 +290,10 @@ static void char_mux_finalize(Object *obj)
> >       bit = -1;
> >       while ((bit = find_next_bit(&d->mux_bitset, MAX_MUX, bit + 1)) <
> MAX_MUX) {
> >           CharBackend *be = d->backends[bit];
> > -        if (be) {
> > -            be->chr = NULL;
> > -        }
> > +        be->chr = NULL;
> > +        d->backends[bit] = NULL;
> >       }
> > +    d->mux_bitset = 0;
> >       qemu_chr_fe_deinit(&d->chr, false);
> >   }
> >
> > @@ -332,6 +332,21 @@ bool mux_chr_attach_frontend(MuxChardev *d,
> CharBackend *b,
> >       return true;
> >   }
> >
> > +bool mux_chr_detach_frontend(MuxChardev *d, unsigned int tag)
> > +{
> > +    unsigned int bit;
> > +
> > +    bit = find_next_bit(&d->mux_bitset, MAX_MUX, tag);
> > +    if (bit != tag) {
> > +        return false;
> > +    }
> > +
> > +    d->mux_bitset &= ~(1 << bit);
>
> mux_bitset is unsigned long, I think we should use 1ul here even id
> MAX_MUX is a low value
>


Sent a patch on that. Thanks.

--
Roman

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