[Qemu-devel] [PATCH v2 0/2] mux chardev events regression fix

[Qemu-devel] [PATCH v2 0/2] mux chardev events regression fix

[Marc-André Lureau]

Hi,

The following patches fix and test the behaviour of mux chardev
events, after a regression introduced in qemu 2.8.0.

v1->v2:
- fix incompatible pointer type warning spotted by patchew

Marc-André Lureau (2):
  chardev: fix backend events regression with mux chardev
  test: add some chardev mux event tests

 include/chardev/char.h |  1 +
 chardev/char-mux.c     |  8 ++++++++
 chardev/char.c         | 18 ++++++++++++------
 tests/test-char.c      | 17 +++++++++++++++++
 4 files changed, 38 insertions(+), 6 deletions(-)

-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PATCH v2 1/2] chardev: fix backend events regression with mux chardev

[Marc-André Lureau]

Kirill noticied that on recent versions on QEMU he was not able to
trigger SysRq to invoke debug capabilites of Linux Kernel.  He tracked
it down to qemu_chr_be_event() ignoring CHR_EVENT_BREAK due s->be
being NULL. The bug was introduced in 2.8, commit a4afa548fc6d ("char:
move front end handlers in CharBackend"). Since the commit, the
qemu_chr_be_event() failed to deliver CHR_EVENT_BREAK due to
qemu_chr_fe_init() does not set s->be in case of mux.

Let's fix this by teaching mux to send an event to the frontend with
the focus.

Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Fixes: a4afa548fc6d ("char: move front end handlers in CharBackend")
---
 include/chardev/char.h |  1 +
 chardev/char-mux.c     |  8 ++++++++
 chardev/char.c         | 18 ++++++++++++------
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 43aabccef5..778d610295 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -248,6 +248,7 @@ typedef struct ChardevClass {
     void (*chr_accept_input)(Chardev *chr);
     void (*chr_set_echo)(Chardev *chr, bool echo);
     void (*chr_set_fe_open)(Chardev *chr, int fe_open);
+    void (*chr_be_event)(Chardev *s, int event);
 } ChardevClass;
 
 Chardev *qemu_chardev_new(const char *id, const char *typename,
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 4cda5e7458..0553b48b90 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -123,6 +123,13 @@ static void mux_chr_send_event(MuxChardev *d, int mux_nr, int event)
     }
 }
 
+static void mux_chr_be_event(Chardev *chr, int event)
+{
+    MuxChardev *d = MUX_CHARDEV(chr);
+
+    mux_chr_send_event(d, d->focus, event);
+}
+
 static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)
 {
     if (d->term_got_escape) {
@@ -346,6 +353,7 @@ static void char_mux_class_init(ObjectClass *oc, void *data)
     cc->chr_write = mux_chr_write;
     cc->chr_accept_input = mux_chr_accept_input;
     cc->chr_add_watch = mux_chr_add_watch;
+    cc->chr_be_event = mux_chr_be_event;
 }
 
 static const TypeInfo char_mux_type_info = {
diff --git a/chardev/char.c b/chardev/char.c
index 2ae4f465ec..8c3765ee99 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -43,10 +43,19 @@ static Object *get_chardevs_root(void)
     return container_get(object_get_root(), "/chardevs");
 }
 
-void qemu_chr_be_event(Chardev *s, int event)
+static void chr_be_event(Chardev *s, int event)
 {
     CharBackend *be = s->be;
 
+    if (!be || !be->chr_event) {
+        return;
+    }
+
+    be->chr_event(be->opaque, event);
+}
+
+void qemu_chr_be_event(Chardev *s, int event)
+{
     /* Keep track if the char device is open */
     switch (event) {
         case CHR_EVENT_OPENED:
@@ -57,11 +66,7 @@ void qemu_chr_be_event(Chardev *s, int event)
             break;
     }
 
-    if (!be || !be->chr_event) {
-        return;
-    }
-
-    be->chr_event(be->opaque, event);
+    CHARDEV_GET_CLASS(s)->chr_be_event(s, event);
 }
 
 /* Not reporting errors from writing to logfile, as logs are
@@ -244,6 +249,7 @@ static void char_class_init(ObjectClass *oc, void *data)
     ChardevClass *cc = CHARDEV_CLASS(oc);
 
     cc->chr_write = null_chr_write;
+    cc->chr_be_event = chr_be_event;
 }
 
 static void char_finalize(Object *obj)
-- 
2.15.0.rc0.40.gaefcc5f6f

[Qemu-devel] [PATCH v2 2/2] test: add some chardev mux event tests

[Marc-André Lureau]

Check the expected behaviour of qemu_chr_be_event() on a mux chardev.

For some reason, sending the event on the base chardev broadcast to
all frontends, while sending it on the mux chardev itself should
trigger the event on the currently focused chardev frontend.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-char.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/test-char.c b/tests/test-char.c
index 7ac25ff73f..911e3f6e8d 100644
--- a/tests/test-char.c
+++ b/tests/test-char.c
@@ -5,6 +5,7 @@
 #include "qemu/config-file.h"
 #include "qemu/sockets.h"
 #include "chardev/char-fe.h"
+#include "chardev/char-mux.h"
 #include "sysemu/sysemu.h"
 #include "qapi/error.h"
 #include "qom/qom-qobject.h"
@@ -164,6 +165,7 @@ static void char_mux_test(void)
     FeHandler h1 = { 0, }, h2 = { 0, };
     CharBackend chr_be1, chr_be2;
 
+    muxes_realized = true; /* done after machine init */
     opts = qemu_opts_create(qemu_find_opts("chardev"), "mux-label",
                             1, &error_abort);
     qemu_opt_set(opts, "backend", "ringbuf", &error_abort);
@@ -201,8 +203,23 @@ static void char_mux_test(void)
     g_assert_cmpstr(h2.read_buf, ==, "hello");
     h2.read_count = 0;
 
+    g_assert_cmpint(h1.last_event, !=, 42); /* should be MUX_OUT or OPENED */
+    g_assert_cmpint(h2.last_event, !=, 42); /* should be MUX_IN or OPENED */
+    /* sending event on the base broadcast to all fe, historical reasons? */
+    qemu_chr_be_event(base, 42);
+    g_assert_cmpint(h1.last_event, ==, 42);
+    g_assert_cmpint(h2.last_event, ==, 42);
+    qemu_chr_be_event(chr, -1);
+    g_assert_cmpint(h1.last_event, ==, 42);
+    g_assert_cmpint(h2.last_event, ==, -1);
+
     /* switch focus */
     qemu_chr_be_write(base, (void *)"\1c", 2);
+    g_assert_cmpint(h1.last_event, ==, CHR_EVENT_MUX_IN);
+    g_assert_cmpint(h2.last_event, ==, CHR_EVENT_MUX_OUT);
+    qemu_chr_be_event(chr, -1);
+    g_assert_cmpint(h1.last_event, ==, -1);
+    g_assert_cmpint(h2.last_event, ==, CHR_EVENT_MUX_OUT);
 
     qemu_chr_be_write(base, (void *)"hello", 6);
     g_assert_cmpint(h2.read_count, ==, 0);
-- 
2.15.0.rc0.40.gaefcc5f6f

Re: [Qemu-devel] [PATCH v2 1/2] chardev: fix backend events regression with mux chardev

[Kirill A. Shutemov]

On Fri, Nov 03, 2017 at 03:28:23PM +0000, Marc-André Lureau wrote:
> Kirill noticied that on recent versions on QEMU he was not able to
> trigger SysRq to invoke debug capabilites of Linux Kernel.  He tracked
> it down to qemu_chr_be_event() ignoring CHR_EVENT_BREAK due s->be
> being NULL. The bug was introduced in 2.8, commit a4afa548fc6d ("char:
> move front end handlers in CharBackend"). Since the commit, the
> qemu_chr_be_event() failed to deliver CHR_EVENT_BREAK due to
> qemu_chr_fe_init() does not set s->be in case of mux.
> 
> Let's fix this by teaching mux to send an event to the frontend with
> the focus.
> 
> Reported-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Fixes: a4afa548fc6d ("char: move front end handlers in CharBackend")

Works for me.

Tested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

Re: [Qemu-devel] [PATCH v2 0/2] mux chardev events regression fix

[Kirill A. Shutemov]

On Fri, Nov 03, 2017 at 03:28:22PM +0000, Marc-André Lureau wrote:
> Hi,
> 
> The following patches fix and test the behaviour of mux chardev
> events, after a regression introduced in qemu 2.8.0.
> 
> v1->v2:
> - fix incompatible pointer type warning spotted by patchew

Looks like this patchset got stuck and never reached upsteam.

Any update on this?

-- 
 Kirill A. Shutemov

Re: [Qemu-devel] [PATCH v2 0/2] mux chardev events regression fix

[Paolo Bonzini]

On 03/11/2017 16:28, Marc-André Lureau wrote:
> Hi,
> 
> The following patches fix and test the behaviour of mux chardev
> events, after a regression introduced in qemu 2.8.0.
> 
> v1->v2:
> - fix incompatible pointer type warning spotted by patchew
> 
> Marc-André Lureau (2):
>   chardev: fix backend events regression with mux chardev
>   test: add some chardev mux event tests
> 
>  include/chardev/char.h |  1 +
>  chardev/char-mux.c     |  8 ++++++++
>  chardev/char.c         | 18 ++++++++++++------
>  tests/test-char.c      | 17 +++++++++++++++++
>  4 files changed, 38 insertions(+), 6 deletions(-)
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH v2 0/2] mux chardev events regression fix

[Paolo Bonzini]

On 20/12/2017 16:11, Paolo Bonzini wrote:
> On 03/11/2017 16:28, Marc-André Lureau wrote:
>> Hi,
>>
>> The following patches fix and test the behaviour of mux chardev
>> events, after a regression introduced in qemu 2.8.0.
>>
>> v1->v2:
>> - fix incompatible pointer type warning spotted by patchew
>>
>> Marc-André Lureau (2):
>>   chardev: fix backend events regression with mux chardev
>>   test: add some chardev mux event tests
>>
>>  include/chardev/char.h |  1 +
>>  chardev/char-mux.c     |  8 ++++++++
>>  chardev/char.c         | 18 ++++++++++++------
>>  tests/test-char.c      | 17 +++++++++++++++++
>>  4 files changed, 38 insertions(+), 6 deletions(-)
>>
> 
> Queued, thanks.

Actually according to Peter's testing this is needed:

diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 0553b48b90..567bf965cd 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -127,7 +127,9 @@ static void mux_chr_be_event(Chardev *chr, int event)
 {
     MuxChardev *d = MUX_CHARDEV(chr);
 
-    mux_chr_send_event(d, d->focus, event);
+    if (d->focus != -1) {
+        mux_chr_send_event(d, d->focus, event);
+    }
 }
 
 static int mux_proc_byte(Chardev *chr, MuxChardev *d, int ch)