[Qemu-devel] [PATCH 0/11] chardev frontend open handling cleanup v2

[Qemu-devel] [PATCH 0/11] chardev frontend open handling cleanup v2

[Hans de Goede]

This patch-series is the result of the
"[PATCH 1/2] char: add qemu_chr_be_is_fe_connected" discussion thread.

This patch series makes the frontend open concept both more explicit and
generic, and significantly cleans up the surrounding code.

Changes in v2:
- Based on top of latest master
- Add protection against double closing / opening to:
  [PATCH 03/11] qemu-char: Add fe_open tracking
- Add 3 new patches, including a fix for the migration issue which started
  the whole discussion:
  [PATCH 07/11] qemu-char: Move incrementing of avail_connections to
  [PATCH 08/11] qemu-char: add_handlers: Don't re-send the be_open
  [PATCH 10/11] virtio-serial: propagate guest_connected to the port on

Regards,

Hans

[Qemu-devel] [PATCH 01/11] qemu-char: Rename opened to be_open

[Hans de Goede]

Rename the opened variable to be_open to reflect that it contains the
opened state of the backend.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/dev-serial.c | 2 +-
 hw/usb/redirect.c   | 2 +-
 include/char/char.h | 2 +-
 qemu-char.c         | 6 +++---
 spice-qemu-char.c   | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/usb/dev-serial.c b/hw/usb/dev-serial.c
index 47ac8c9..7c314dc 100644
--- a/hw/usb/dev-serial.c
+++ b/hw/usb/dev-serial.c
@@ -495,7 +495,7 @@ static int usb_serial_initfn(USBDevice *dev)
                           usb_serial_event, s);
     usb_serial_handle_reset(dev);
 
-    if (s->cs->opened && !dev->attached) {
+    if (s->cs->be_open && !dev->attached) {
         usb_device_attach(dev);
     }
     return 0;
diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index c519b9b..9734e42 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -258,7 +258,7 @@ static int usbredir_write(void *priv, uint8_t *data, int count)
 {
     USBRedirDevice *dev = priv;
 
-    if (!dev->cs->opened) {
+    if (!dev->cs->be_open) {
         return 0;
     }
 
diff --git a/include/char/char.h b/include/char/char.h
index 0326b2a..d801f92 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -74,7 +74,7 @@ struct CharDriverState {
     int idle_tag;
     char *label;
     char *filename;
-    int opened;
+    int be_open;
     int avail_connections;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
diff --git a/qemu-char.c b/qemu-char.c
index 4e011df..32a05af 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -108,10 +108,10 @@ void qemu_chr_be_event(CharDriverState *s, int event)
     /* Keep track if the char device is open */
     switch (event) {
         case CHR_EVENT_OPENED:
-            s->opened = 1;
+            s->be_open = 1;
             break;
         case CHR_EVENT_CLOSED:
-            s->opened = 0;
+            s->be_open = 0;
             break;
     }
 
@@ -207,7 +207,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
 
     /* We're connecting to an already opened device, so let's make sure we
        also get the open event */
-    if (s->opened) {
+    if (s->be_open) {
         qemu_chr_generic_open(s);
     }
 }
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 8a9236d..c39095b 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -100,8 +100,8 @@ static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
     }
 #endif
 
-    if ((scd->chr->opened && connected) ||
-        (!scd->chr->opened && !connected)) {
+    if ((scd->chr->be_open && connected) ||
+        (!scd->chr->be_open && !connected)) {
         return;
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 02/11] qemu-char: Rename qemu_chr_generic_open to qemu_chr_be_generic_open

[Hans de Goede]

To better reflect that it is for handling a backend being opened.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 backends/baum.c     |  2 +-
 include/char/char.h |  2 +-
 qemu-char.c         | 24 ++++++++++++------------
 ui/console.c        |  2 +-
 ui/gtk.c            |  2 +-
 5 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/backends/baum.c b/backends/baum.c
index d7d658c..ea9ffe8 100644
--- a/backends/baum.c
+++ b/backends/baum.c
@@ -611,7 +611,7 @@ CharDriverState *chr_baum_init(void)
 
     qemu_set_fd_handler(baum->brlapi_fd, baum_chr_read, NULL, baum);
 
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
 
     return chr;
 
diff --git a/include/char/char.h b/include/char/char.h
index d801f92..dd8f39a 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -235,7 +235,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
                            IOEventHandler *fd_event,
                            void *opaque);
 
-void qemu_chr_generic_open(CharDriverState *s);
+void qemu_chr_be_generic_open(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
 int qemu_chr_add_client(CharDriverState *s, int fd);
 void qemu_chr_info_print(Monitor *mon, const QObject *ret_data);
diff --git a/qemu-char.c b/qemu-char.c
index 32a05af..55795d7 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -120,7 +120,7 @@ void qemu_chr_be_event(CharDriverState *s, int event)
     s->chr_event(s->handler_opaque, event);
 }
 
-static gboolean qemu_chr_generic_open_bh(gpointer opaque)
+static gboolean qemu_chr_be_generic_open_bh(gpointer opaque)
 {
     CharDriverState *s = opaque;
     qemu_chr_be_event(s, CHR_EVENT_OPENED);
@@ -128,10 +128,10 @@ static gboolean qemu_chr_generic_open_bh(gpointer opaque)
     return FALSE;
 }
 
-void qemu_chr_generic_open(CharDriverState *s)
+void qemu_chr_be_generic_open(CharDriverState *s)
 {
     if (s->idle_tag == 0) {
-        s->idle_tag = g_idle_add(qemu_chr_generic_open_bh, s);
+        s->idle_tag = g_idle_add(qemu_chr_be_generic_open_bh, s);
     }
 }
 
@@ -208,7 +208,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
     /* We're connecting to an already opened device, so let's make sure we
        also get the open event */
     if (s->be_open) {
-        qemu_chr_generic_open(s);
+        qemu_chr_be_generic_open(s);
     }
 }
 
@@ -482,7 +482,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     chr->chr_guest_close = NULL;
 
     /* Muxes are always open on creation */
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
 
     return chr;
 }
@@ -836,7 +836,7 @@ static CharDriverState *qemu_chr_open_fd(int fd_in, int fd_out)
     chr->chr_update_read_handler = fd_chr_update_read_handler;
     chr->chr_close = fd_chr_close;
 
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
 
     return chr;
 }
@@ -1133,7 +1133,7 @@ static void pty_chr_state(CharDriverState *chr, int connected)
         pty_chr_rearm_timer(chr, 1000);
     } else {
         if (!s->connected)
-            qemu_chr_generic_open(chr);
+            qemu_chr_be_generic_open(chr);
         s->connected = 1;
     }
 }
@@ -1549,7 +1549,7 @@ static CharDriverState *qemu_chr_open_pp_fd(int fd)
     chr->chr_close = pp_close;
     chr->opaque = drv;
 
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
 
     return chr;
 }
@@ -1834,7 +1834,7 @@ static CharDriverState *qemu_chr_open_win_path(const char *filename)
         g_free(chr);
         return NULL;
     }
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
     return chr;
 }
 
@@ -1934,7 +1934,7 @@ static CharDriverState *qemu_chr_open_pipe(ChardevHostdev *opts)
         g_free(chr);
         return NULL;
     }
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
     return chr;
 }
 
@@ -1948,7 +1948,7 @@ static CharDriverState *qemu_chr_open_win_file(HANDLE fd_out)
     s->hcom = fd_out;
     chr->opaque = s;
     chr->chr_write = win_chr_write;
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
     return chr;
 }
 
@@ -2513,7 +2513,7 @@ static void tcp_chr_connect(void *opaque)
     if (s->chan) {
         s->tag = io_add_watch_poll(s->chan, tcp_chr_read_poll, tcp_chr_read, chr);
     }
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
 }
 
 #define IACSET(x,a,b,c) x[0] = a; x[1] = b; x[2] = c;
diff --git a/ui/console.c b/ui/console.c
index eb7a2bc..e84ba8b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1600,7 +1600,7 @@ static void text_console_do_init(CharDriverState *chr, DisplayState *ds)
         s->t_attrib = s->t_attrib_default;
     }
 
-    qemu_chr_generic_open(chr);
+    qemu_chr_be_generic_open(chr);
     if (chr->init)
         chr->init(chr);
 }
diff --git a/ui/gtk.c b/ui/gtk.c
index 1edfaca..a5a8156 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1163,7 +1163,7 @@ static GSList *gd_vc_init(GtkDisplayState *s, VirtualConsole *vc, int index, GSL
 
     gtk_menu_shell_append(GTK_MENU_SHELL(s->view_menu), vc->menu_item);
 
-    qemu_chr_generic_open(vc->chr);
+    qemu_chr_be_generic_open(vc->chr);
     if (vc->chr->init) {
         vc->chr->init(vc->chr);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 03/11] qemu-char: Add fe_open tracking

[Hans de Goede]

Add tracking of the fe_open state to struct CharDriverState.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/char/char.h | 1 +
 qemu-char.c         | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/char/char.h b/include/char/char.h
index dd8f39a..3174575 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -75,6 +75,7 @@ struct CharDriverState {
     char *label;
     char *filename;
     int be_open;
+    int fe_open;
     int avail_connections;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
diff --git a/qemu-char.c b/qemu-char.c
index 55795d7..2f35504 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3385,6 +3385,10 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
 
 void qemu_chr_fe_open(struct CharDriverState *chr)
 {
+    if (chr->fe_open) {
+        return;
+    }
+    chr->fe_open = 1;
     if (chr->chr_guest_open) {
         chr->chr_guest_open(chr);
     }
@@ -3392,6 +3396,10 @@ void qemu_chr_fe_open(struct CharDriverState *chr)
 
 void qemu_chr_fe_close(struct CharDriverState *chr)
 {
+    if (!chr->fe_open) {
+        return;
+    }
+    chr->fe_open = 0;
     if (chr->chr_guest_close) {
         chr->chr_guest_close(chr);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 04/11] qemu-char: Automatically do fe_open / fe_close on qemu_chr_add_handlers

[Hans de Goede]

Most frontends can't really determine if the guest actually has the frontend
side open. So lets automatically generate fe_open / fe_close as soon as a
frontend becomes ready (as signalled by calling qemu_chr_add_handlers) /
becomes non ready (as signalled by setting all handlers to NULL).

And allow frontends which can actually determine if the guest is listening to
opt-out of this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/usb/redirect.c   |  2 --
 hw/virtio-console.c |  1 +
 include/char/char.h |  1 +
 qemu-char.c         | 13 +++++++++++++
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 9734e42..d02a7b9 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1282,7 +1282,6 @@ static int usbredir_initfn(USBDevice *udev)
     dev->compatible_speedmask = USB_SPEED_MASK_FULL | USB_SPEED_MASK_HIGH;
 
     /* Let the backend know we are ready */
-    qemu_chr_fe_open(dev->cs);
     qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
                           usbredir_chardev_read, usbredir_chardev_event, dev);
 
@@ -1306,7 +1305,6 @@ static void usbredir_handle_destroy(USBDevice *udev)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
-    qemu_chr_fe_close(dev->cs);
     qemu_chr_delete(dev->cs);
     /* Note must be done after qemu_chr_close, as that causes a close event */
     qemu_bh_delete(dev->chardev_close_bh);
diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index e2d1c58..2f7c3df 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -131,6 +131,7 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
     }
 
     if (vcon->chr) {
+        vcon->chr->explicit_fe_open = 1;
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
     }
diff --git a/include/char/char.h b/include/char/char.h
index 3174575..27ebbc3 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -76,6 +76,7 @@ struct CharDriverState {
     char *filename;
     int be_open;
     int fe_open;
+    int explicit_fe_open;
     int avail_connections;
     QemuOpts *opts;
     QTAILQ_ENTRY(CharDriverState) next;
diff --git a/qemu-char.c b/qemu-char.c
index 2f35504..f580297 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -194,9 +194,14 @@ void qemu_chr_add_handlers(CharDriverState *s,
                            IOEventHandler *fd_event,
                            void *opaque)
 {
+    int fe_open;
+
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         /* chr driver being released. */
         ++s->avail_connections;
+        fe_open = 0;
+    } else {
+        fe_open = 1;
     }
     s->chr_can_read = fd_can_read;
     s->chr_read = fd_read;
@@ -205,6 +210,14 @@ void qemu_chr_add_handlers(CharDriverState *s,
     if (s->chr_update_read_handler)
         s->chr_update_read_handler(s);
 
+    if (!s->explicit_fe_open) {
+        if (fe_open) {
+            qemu_chr_fe_open(s);
+        } else {
+            qemu_chr_fe_close(s);
+        }
+    }
+
     /* We're connecting to an already opened device, so let's make sure we
        also get the open event */
     if (s->be_open) {
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/11] qemu-char: Cleanup: consolidate fe_open/fe_close into fe_set_open

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-console.c |  4 ++--
 include/char/char.h | 17 ++++-------------
 qemu-char.c         | 24 ++++++------------------
 3 files changed, 12 insertions(+), 33 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 2f7c3df..7c89990 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -74,7 +74,7 @@ static void guest_open(VirtIOSerialPort *port)
     if (!vcon->chr) {
         return;
     }
-    qemu_chr_fe_open(vcon->chr);
+    qemu_chr_fe_set_open(vcon->chr, 1);
 }
 
 /* Callback function that's called when the guest closes the port */
@@ -85,7 +85,7 @@ static void guest_close(VirtIOSerialPort *port)
     if (!vcon->chr) {
         return;
     }
-    qemu_chr_fe_close(vcon->chr);
+    qemu_chr_fe_set_open(vcon->chr, 0);
 }
 
 /* Readiness of the guest to accept data on a port */
diff --git a/include/char/char.h b/include/char/char.h
index 27ebbc3..3c8dd28 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -129,21 +129,12 @@ void qemu_chr_delete(CharDriverState *chr);
 void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo);
 
 /**
- * @qemu_chr_fe_open:
+ * @qemu_chr_fe_set_open:
  *
- * Open a character backend.  This function call is an indication that the
- * front end is ready to begin doing I/O.
+ * Set character frontend open status.  This is an indication that the
+ * front end is ready (or not) to begin doing I/O.
  */
-void qemu_chr_fe_open(struct CharDriverState *chr);
-
-/**
- * @qemu_chr_fe_close:
- *
- * Close a character backend.  This function call indicates that the front end
- * no longer is able to process I/O.  To process I/O again, the front end will
- * call @qemu_chr_fe_open.
- */
-void qemu_chr_fe_close(struct CharDriverState *chr);
+void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open);
 
 /**
  * @qemu_chr_fe_printf:
diff --git a/qemu-char.c b/qemu-char.c
index f580297..3651af1 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -211,11 +211,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
         s->chr_update_read_handler(s);
 
     if (!s->explicit_fe_open) {
-        if (fe_open) {
-            qemu_chr_fe_open(s);
-        } else {
-            qemu_chr_fe_close(s);
-        }
+        qemu_chr_fe_set_open(s, fe_open);
     }
 
     /* We're connecting to an already opened device, so let's make sure we
@@ -3396,24 +3392,16 @@ void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo)
     }
 }
 
-void qemu_chr_fe_open(struct CharDriverState *chr)
+void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open)
 {
-    if (chr->fe_open) {
+    if (chr->fe_open == fe_open) {
         return;
     }
-    chr->fe_open = 1;
-    if (chr->chr_guest_open) {
+    chr->fe_open = fe_open;
+    if (fe_open && chr->chr_guest_open) {
         chr->chr_guest_open(chr);
     }
-}
-
-void qemu_chr_fe_close(struct CharDriverState *chr)
-{
-    if (!chr->fe_open) {
-        return;
-    }
-    chr->fe_open = 0;
-    if (chr->chr_guest_close) {
+    if (!fe_open && chr->chr_guest_close) {
         chr->chr_guest_close(chr);
     }
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/11] qemu-char: Consolidate guest_close/guest_open into a set_fe_open callback

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/char/char.h |  3 +--
 qemu-char.c         | 10 +++-------
 spice-qemu-char.c   | 17 +++++++----------
 3 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/include/char/char.h b/include/char/char.h
index 3c8dd28..1457e80 100644
--- a/include/char/char.h
+++ b/include/char/char.h
@@ -68,8 +68,7 @@ struct CharDriverState {
     void (*chr_close)(struct CharDriverState *chr);
     void (*chr_accept_input)(struct CharDriverState *chr);
     void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
-    void (*chr_guest_open)(struct CharDriverState *chr);
-    void (*chr_guest_close)(struct CharDriverState *chr);
+    void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
     void *opaque;
     int idle_tag;
     char *label;
diff --git a/qemu-char.c b/qemu-char.c
index 3651af1..8a66627 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -487,8 +487,7 @@ static CharDriverState *qemu_chr_open_mux(CharDriverState *drv)
     chr->chr_update_read_handler = mux_chr_update_read_handler;
     chr->chr_accept_input = mux_chr_accept_input;
     /* Frontend guest-open / -close notification is not support with muxes */
-    chr->chr_guest_open = NULL;
-    chr->chr_guest_close = NULL;
+    chr->chr_set_fe_open = NULL;
 
     /* Muxes are always open on creation */
     qemu_chr_be_generic_open(chr);
@@ -3398,11 +3397,8 @@ void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open)
         return;
     }
     chr->fe_open = fe_open;
-    if (fe_open && chr->chr_guest_open) {
-        chr->chr_guest_open(chr);
-    }
-    if (!fe_open && chr->chr_guest_close) {
-        chr->chr_guest_close(chr);
+    if (chr->chr_set_fe_open) {
+        chr->chr_set_fe_open(chr, fe_open);
     }
 }
 
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index c39095b..d249829 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -176,16 +176,14 @@ static void spice_chr_close(struct CharDriverState *chr)
     g_free(s);
 }
 
-static void spice_chr_guest_open(struct CharDriverState *chr)
+static void spice_chr_set_fe_open(struct CharDriverState *chr, int fe_open)
 {
     SpiceCharDriver *s = chr->opaque;
-    vmc_register_interface(s);
-}
-
-static void spice_chr_guest_close(struct CharDriverState *chr)
-{
-    SpiceCharDriver *s = chr->opaque;
-    vmc_unregister_interface(s);
+    if (fe_open) {
+        vmc_register_interface(s);
+    } else {
+        vmc_unregister_interface(s);
+    }
 }
 
 static void print_allowed_subtypes(void)
@@ -218,8 +216,7 @@ static CharDriverState *chr_open(const char *subtype)
     chr->opaque = s;
     chr->chr_write = spice_chr_write;
     chr->chr_close = spice_chr_close;
-    chr->chr_guest_open = spice_chr_guest_open;
-    chr->chr_guest_close = spice_chr_guest_close;
+    chr->chr_set_fe_open = spice_chr_set_fe_open;
 
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Hans de Goede]

The decrement of avail_connections is done in qdev-properties-system move
the increment there too for proper balancing of the calls.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/qdev-properties-system.c | 6 ++++--
 qemu-char.c                 | 2 --
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
index 8795144..12a87d5 100644
--- a/hw/qdev-properties-system.c
+++ b/hw/qdev-properties-system.c
@@ -136,9 +136,11 @@ static void release_chr(Object *obj, const char *name, void *opaque)
     DeviceState *dev = DEVICE(obj);
     Property *prop = opaque;
     CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
+    CharDriverState *chr = *ptr;
 
-    if (*ptr) {
-        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
+    if (chr) {
+        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
+        ++chr->avail_connections;
     }
 }
 
diff --git a/qemu-char.c b/qemu-char.c
index 8a66627..368e7f5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -197,8 +197,6 @@ void qemu_chr_add_handlers(CharDriverState *s,
     int fe_open;
 
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
-        /* chr driver being released. */
-        ++s->avail_connections;
         fe_open = 0;
     } else {
         fe_open = 1;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Paolo Bonzini]

Il 26/03/2013 11:07, Hans de Goede ha scritto:
> The decrement of avail_connections is done in qdev-properties-system move
> the increment there too for proper balancing of the calls.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  hw/qdev-properties-system.c | 6 ++++--
>  qemu-char.c                 | 2 --
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
> index 8795144..12a87d5 100644
> --- a/hw/qdev-properties-system.c
> +++ b/hw/qdev-properties-system.c
> @@ -136,9 +136,11 @@ static void release_chr(Object *obj, const char *name, void *opaque)
>      DeviceState *dev = DEVICE(obj);
>      Property *prop = opaque;
>      CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> +    CharDriverState *chr = *ptr;
>  
> -    if (*ptr) {
> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> +    if (chr) {
> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
> +        ++chr->avail_connections;
>      }
>  }
>  
> diff --git a/qemu-char.c b/qemu-char.c
> index 8a66627..368e7f5 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -197,8 +197,6 @@ void qemu_chr_add_handlers(CharDriverState *s,
>      int fe_open;
>  
>      if (!opaque && !fd_can_read && !fd_read && !fd_event) {
> -        /* chr driver being released. */
> -        ++s->avail_connections;
>          fe_open = 0;
>      } else {
>          fe_open = 1;
> 

I think this is still wrong (though better than before this patch).
This is still not giving an error:

   qemu-kvm \
      -chardev stdio,id=foo -device isa-serial,chardev=foo \
      -mon chardev=foo

because other users of -chardev (monitor and rng-egd), are not
decrementing avail_connections.  Can you look at it in a follow-up?

Paolo

Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Hans de Goede]

Hi,

On 03/26/2013 02:07 PM, Paolo Bonzini wrote:
> Il 26/03/2013 11:07, Hans de Goede ha scritto:
>> The decrement of avail_connections is done in qdev-properties-system move
>> the increment there too for proper balancing of the calls.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   hw/qdev-properties-system.c | 6 ++++--
>>   qemu-char.c                 | 2 --
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
>> index 8795144..12a87d5 100644
>> --- a/hw/qdev-properties-system.c
>> +++ b/hw/qdev-properties-system.c
>> @@ -136,9 +136,11 @@ static void release_chr(Object *obj, const char *name, void *opaque)
>>       DeviceState *dev = DEVICE(obj);
>>       Property *prop = opaque;
>>       CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>> +    CharDriverState *chr = *ptr;
>>
>> -    if (*ptr) {
>> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
>> +    if (chr) {
>> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>> +        ++chr->avail_connections;
>>       }
>>   }
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index 8a66627..368e7f5 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -197,8 +197,6 @@ void qemu_chr_add_handlers(CharDriverState *s,
>>       int fe_open;
>>
>>       if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>> -        /* chr driver being released. */
>> -        ++s->avail_connections;
>>           fe_open = 0;
>>       } else {
>>           fe_open = 1;
>>
>
> I think this is still wrong (though better than before this patch).
> This is still not giving an error:
>
>     qemu-kvm \
>        -chardev stdio,id=foo -device isa-serial,chardev=foo \
>        -mon chardev=foo
>
> because other users of -chardev (monitor and rng-egd), are not
> decrementing avail_connections.  Can you look at it in a follow-up?

I know, I ended up writing this patch mostly as a side-effect.

I can put further fixing this on my TODO list but first I've some
questions about this which need answering:

1) For most problematic devices, the proper fix would be to make them
use a chardev qdev property for there chardev usage, and then this
would be automatically fixed, agreed?

2) For some this may not fly and a manual inc / dec of avail_connections
is necessary, ie the monitor, agreed?

3) One weird case which I encountered when working on this I noticed
that backends/rng-egd.c has its chardev as a string qdev-property, rather
then as a chardev qdev-property and then it does a qemu_chr_find itself,
is this intentional, iow is there some reason having it as a
chardev qdev-property does not work ?

Regards,

Hans

Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Paolo Bonzini]

Il 26/03/2013 14:30, Hans de Goede ha scritto:
> Hi,
> 
> On 03/26/2013 02:07 PM, Paolo Bonzini wrote:
>> Il 26/03/2013 11:07, Hans de Goede ha scritto:
>>> The decrement of avail_connections is done in qdev-properties-system
>>> move
>>> the increment there too for proper balancing of the calls.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   hw/qdev-properties-system.c | 6 ++++--
>>>   qemu-char.c                 | 2 --
>>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
>>> index 8795144..12a87d5 100644
>>> --- a/hw/qdev-properties-system.c
>>> +++ b/hw/qdev-properties-system.c
>>> @@ -136,9 +136,11 @@ static void release_chr(Object *obj, const char
>>> *name, void *opaque)
>>>       DeviceState *dev = DEVICE(obj);
>>>       Property *prop = opaque;
>>>       CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>>> +    CharDriverState *chr = *ptr;
>>>
>>> -    if (*ptr) {
>>> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
>>> +    if (chr) {
>>> +        qemu_chr_add_handlers(chr, NULL, NULL, NULL, NULL);
>>> +        ++chr->avail_connections;
>>>       }
>>>   }
>>>
>>> diff --git a/qemu-char.c b/qemu-char.c
>>> index 8a66627..368e7f5 100644
>>> --- a/qemu-char.c
>>> +++ b/qemu-char.c
>>> @@ -197,8 +197,6 @@ void qemu_chr_add_handlers(CharDriverState *s,
>>>       int fe_open;
>>>
>>>       if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>>> -        /* chr driver being released. */
>>> -        ++s->avail_connections;
>>>           fe_open = 0;
>>>       } else {
>>>           fe_open = 1;
>>>
>>
>> I think this is still wrong (though better than before this patch).
>> This is still not giving an error:
>>
>>     qemu-kvm \
>>        -chardev stdio,id=foo -device isa-serial,chardev=foo \
>>        -mon chardev=foo
>>
>> because other users of -chardev (monitor and rng-egd), are not
>> decrementing avail_connections.  Can you look at it in a follow-up?
> 
> I know, I ended up writing this patch mostly as a side-effect.
> 
> I can put further fixing this on my TODO list but first I've some
> questions about this which need answering:
> 
> 1) For most problematic devices, the proper fix would be to make them
> use a chardev qdev property for there chardev usage, and then this
> would be automatically fixed, agreed?

At least on x86, all devices already use a chardev qdev property.

> 2) For some this may not fly and a manual inc / dec of avail_connections
> is necessary, ie the monitor, agreed?
> 
> 3) One weird case which I encountered when working on this I noticed
> that backends/rng-egd.c has its chardev as a string qdev-property, rather
> then as a chardev qdev-property and then it does a qemu_chr_find itself,
> is this intentional, iow is there some reason having it as a
> chardev qdev-property does not work ?

The infrastructure for chardev qdev properties right now is only used
within devices.  The right thing to do would be to make chardevs QOM
objects.  Then you do not need any special code, just make chardevs QOM
links.

Paolo

Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Hans de Goede]

Hi,

On 03/26/2013 02:50 PM, Paolo Bonzini wrote:

<snip>

>> 1) For most problematic devices, the proper fix would be to make them
>> use a chardev qdev property for there chardev usage, and then this
>> would be automatically fixed, agreed?
>
> At least on x86, all devices already use a chardev qdev property.

Yes on x86 maybe, but a lot of the other serial-port emulations are
still using serial_hds directly, making proper avail_connections tracking
a pain.

Anyways I've audited all frontends now, fixing things where necessary,
and where possible in a generic way.

I'll send a patch for this right after this mail.

Regards,

Hans

Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Paolo Bonzini]

Il 27/03/2013 15:09, Hans de Goede ha scritto:
> Hi,
> 
> On 03/26/2013 02:50 PM, Paolo Bonzini wrote:
> 
> <snip>
> 
>>> 1) For most problematic devices, the proper fix would be to make them
>>> use a chardev qdev property for there chardev usage, and then this
>>> would be automatically fixed, agreed?
>>
>> At least on x86, all devices already use a chardev qdev property.
> 
> Yes on x86 maybe, but a lot of the other serial-port emulations are
> still using serial_hds directly, making proper avail_connections tracking
> a pain.

serial_hds is still passed to most devices via a chardev qdev property.
 See for example sparc/leon3.c, which uses grlib_apbuart_create and that
function sets the chardev.

Luckily there are very few UART implementations, most boards use the
8250/16550, hence this is even true of boards that are generally not
qdev-ified (like OMAP).  There are exceptions, like mcf_uart.c and
bt-hci-csr.c.

Paolo

> Anyways I've audited all frontends now, fixing things where necessary,
> and where possible in a generic way.
> 
> I'll send a patch for this right after this mail.
> 
> Regards,
> 
> Hans

Re: [Qemu-devel] [PATCH 07/11] qemu-char: Move incrementing of avail_connections to qdev-properties-system

[Hans de Goede]

Hi,

On 03/27/2013 03:58 PM, Paolo Bonzini wrote:
> Il 27/03/2013 15:09, Hans de Goede ha scritto:
>> Hi,
>>
>> On 03/26/2013 02:50 PM, Paolo Bonzini wrote:
>>
>> <snip>
>>
>>>> 1) For most problematic devices, the proper fix would be to make them
>>>> use a chardev qdev property for there chardev usage, and then this
>>>> would be automatically fixed, agreed?
>>>
>>> At least on x86, all devices already use a chardev qdev property.
>>
>> Yes on x86 maybe, but a lot of the other serial-port emulations are
>> still using serial_hds directly, making proper avail_connections tracking
>> a pain.
>
> serial_hds is still passed to most devices via a chardev qdev property.

Most, yes but not all, which is why I wrote "using serial_hds *directly*",
anyways see the patch which I send a while back which tries to deal with
all the *direct* serial_hds users, as well as with the monitor, and some
code which does chardev creation completely on its own.

Regards,

Hans

[Qemu-devel] [PATCH 08/11] qemu-char: add_handlers: Don't re-send the be_open event on unregister

[Hans de Goede]

Resending the be_open event only is useful when a frontend is registering, not
when it is unregistering.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 qemu-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index 368e7f5..edf3779 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -214,7 +214,7 @@ void qemu_chr_add_handlers(CharDriverState *s,
 
     /* We're connecting to an already opened device, so let's make sure we
        also get the open event */
-    if (s->be_open) {
+    if (fe_open && s->be_open) {
         qemu_chr_be_generic_open(s);
     }
 }
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/11] virtio-serial: Consolidate guest_open/guest_close into set_guest_connected

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-console.c    | 23 +++++------------------
 hw/virtio-serial-bus.c | 15 +++++----------
 hw/virtio-serial.h     |  6 ++----
 3 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 7c89990..284180f 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -66,26 +66,15 @@ static ssize_t flush_buf(VirtIOSerialPort *port, const uint8_t *buf, size_t len)
     return ret;
 }
 
-/* Callback function that's called when the guest opens the port */
-static void guest_open(VirtIOSerialPort *port)
+/* Callback function that's called when the guest opens/closes the port */
+static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
 {
     VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
 
     if (!vcon->chr) {
         return;
     }
-    qemu_chr_fe_set_open(vcon->chr, 1);
-}
-
-/* Callback function that's called when the guest closes the port */
-static void guest_close(VirtIOSerialPort *port)
-{
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
-
-    if (!vcon->chr) {
-        return;
-    }
-    qemu_chr_fe_set_open(vcon->chr, 0);
+    qemu_chr_fe_set_open(vcon->chr, guest_connected);
 }
 
 /* Readiness of the guest to accept data on a port */
@@ -152,8 +141,7 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
     k->is_console = true;
     k->init = virtconsole_initfn;
     k->have_data = flush_buf;
-    k->guest_open = guest_open;
-    k->guest_close = guest_close;
+    k->set_guest_connected = set_guest_connected;
     dc->props = virtconsole_properties;
 }
 
@@ -176,8 +164,7 @@ static void virtserialport_class_init(ObjectClass *klass, void *data)
 
     k->init = virtconsole_initfn;
     k->have_data = flush_buf;
-    k->guest_open = guest_open;
-    k->guest_close = guest_close;
+    k->set_guest_connected = set_guest_connected;
     dc->props = virtserialport_properties;
 }
 
diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index ab7168e..eb7af21 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -372,14 +372,9 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
 
     case VIRTIO_CONSOLE_PORT_OPEN:
         port->guest_connected = cpkt.value;
-        if (cpkt.value && vsc->guest_open) {
+        if (vsc->set_guest_connected) {
             /* Send the guest opened notification if an app is interested */
-            vsc->guest_open(port);
-        }
-
-        if (!cpkt.value && vsc->guest_close) {
-            /* Send the guest closed notification if an app is interested */
-            vsc->guest_close(port);
+            vsc->set_guest_connected(port, cpkt.value);
         }
         break;
     }
@@ -484,9 +479,9 @@ static void guest_reset(VirtIOSerial *vser)
         vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
         if (port->guest_connected) {
             port->guest_connected = false;
-
-            if (vsc->guest_close)
-                vsc->guest_close(port);
+            if (vsc->set_guest_connected) {
+                vsc->set_guest_connected(port, false);
+            }
         }
     }
 }
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index 484dcfe..516400f 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -92,10 +92,8 @@ typedef struct VirtIOSerialPortClass {
     int (*exit)(VirtIOSerialPort *port);
 
     /* Callbacks for guest events */
-        /* Guest opened device. */
-    void (*guest_open)(VirtIOSerialPort *port);
-        /* Guest closed device. */
-    void (*guest_close)(VirtIOSerialPort *port);
+        /* Guest opened/closed device. */
+    void (*set_guest_connected)(VirtIOSerialPort *port, int guest_connected);
 
         /* Guest is now ready to accept data (virtqueues set up). */
     void (*guest_ready)(VirtIOSerialPort *port);
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/11] virtio-serial: propagate guest_connected to the port on post_load

[Hans de Goede]

From: Alon Levy <alevy@redhat.com>

When migrating a host with with a spice agent running the mouse becomes
non operational after the migration due to the agent state being
inconsistent between the guest and the client.

After migration the spicevmc backend on the destination has never been notified
of the (non 0) guest_connected state. Virtio-serial holds this state
information and migrates it, this patch properly propagates this information
to virtio-console and through that to interested chardev backends.

rhbz #725965

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 hw/virtio-serial-bus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index eb7af21..a9cb114 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -579,6 +579,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
     VirtIOSerial *s = opaque;
     VirtIOSerialPort *port;
     uint8_t host_connected;
+    VirtIOSerialPortClass *vsc;
 
     if (!s->post_load) {
         return;
@@ -594,6 +595,10 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
             send_control_event(s, port->id, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
+        vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+        if (vsc->set_guest_connected) {
+            vsc->set_guest_connected(port, port->guest_connected);
+        }
     }
     g_free(s->post_load->connected);
     qemu_free_timer(s->post_load->timer);
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/11] spice-qemu-char: Drop hackish vmc_register on spice_chr_write

[Hans de Goede]

Now that the core takes care of fe_open tracking we no longer need this hack.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index d249829..535f955 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -149,7 +149,6 @@ static int spice_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
 {
     SpiceCharDriver *s = chr->opaque;
 
-    vmc_register_interface(s);
     assert(s->datalen == 0);
     if (s->bufsize < len) {
         s->bufsize = len;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 0/11] chardev frontend open handling cleanup v2

[Eric Blake]

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

On 03/26/2013 04:07 AM, Hans de Goede wrote:
> This patch-series is the result of the
> "[PATCH 1/2] char: add qemu_chr_be_is_fe_connected" discussion thread.
> 
> This patch series makes the frontend open concept both more explicit and
> generic, and significantly cleans up the surrounding code.
> 
> Changes in v2:
> - Based on top of latest master
> - Add protection against double closing / opening to:
>   [PATCH 03/11] qemu-char: Add fe_open tracking
> - Add 3 new patches, including a fix for the migration issue which started
>   the whole discussion:
>   [PATCH 07/11] qemu-char: Move incrementing of avail_connections to
>   [PATCH 08/11] qemu-char: add_handlers: Don't re-send the be_open
>   [PATCH 10/11] virtio-serial: propagate guest_connected to the port on

Can you convince git to put a diffstat in your cover letters?  It may be
as simple as ensuring 'diffstat' is installed on the machine where you
generate your patch series.  Seeing an overview of the files touched
aids in helping others decide whether they can review the series,
without having to read the entire series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH 0/11] chardev frontend open handling cleanup v2

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori