[Spice-devel,02/12] char: add qemu_chr_fe_event()

Submitted by Marc-André Lureau on June 20, 2013, 5:46 p.m.

Details

Message ID 1371750371-18491-3-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marc-André Lureau June 20, 2013, 5:46 p.m.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/char.h | 10 ++++++++++
 qemu-char.c           |  7 +++++++
 spice-qemu-char.c     | 10 ++++++++++
 3 files changed, 27 insertions(+)

Patch hide | download patch | download mbox

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 066c216..eee70fe 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -69,6 +69,7 @@  struct CharDriverState {
     void (*chr_accept_input)(struct CharDriverState *chr);
     void (*chr_set_echo)(struct CharDriverState *chr, bool echo);
     void (*chr_set_fe_open)(struct CharDriverState *chr, int fe_open);
+    void (*chr_fe_event)(struct CharDriverState *chr, int event);
     void *opaque;
     char *label;
     char *filename;
@@ -136,6 +137,15 @@  void qemu_chr_fe_set_echo(struct CharDriverState *chr, bool echo);
 void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open);
 
 /**
+ * @qemu_chr_fe_event:
+ *
+ * Send an event from the back end to the front end.
+ *
+ * @event the event to send
+ */
+void qemu_chr_fe_event(CharDriverState *s, int event);
+
+/**
  * @qemu_chr_fe_printf:
  *
  * Write to a character backend using a printf style interface.
diff --git a/qemu-char.c b/qemu-char.c
index 2c3cfe6..14e268e 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3317,6 +3317,13 @@  void qemu_chr_fe_set_open(struct CharDriverState *chr, int fe_open)
     }
 }
 
+void qemu_chr_fe_event(struct CharDriverState *chr, int event)
+{
+    if (chr->chr_fe_event) {
+        chr->chr_fe_event(chr, event);
+    }
+}
+
 int qemu_chr_fe_add_watch(CharDriverState *s, GIOCondition cond,
                           GIOFunc func, void *user_data)
 {
diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 6d147a7..0d77f77 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -223,6 +223,15 @@  static void spice_chr_set_fe_open(struct CharDriverState *chr, int fe_open)
     }
 }
 
+static void spice_chr_fe_event(struct CharDriverState *chr, int event)
+{
+#if SPICE_SERVER_VERSION >= 0x000c02
+    SpiceCharDriver *s = chr->opaque;
+
+    spice_server_port_event(&s->sin, event);
+#endif
+}
+
 static void print_allowed_subtypes(void)
 {
     const char** psubtype;
@@ -256,6 +265,7 @@  static CharDriverState *chr_open(const char *subtype)
     chr->chr_close = spice_chr_close;
     chr->chr_set_fe_open = spice_chr_set_fe_open;
     chr->explicit_be_open = true;
+    chr->chr_fe_event = spice_chr_fe_event;
 
     QLIST_INSERT_HEAD(&spice_chars, s, next);
 

Comments

Hi,

> +static void spice_chr_fe_event(struct CharDriverState *chr, int event)
> +{
> +#if SPICE_SERVER_VERSION >= 0x000c02
> +    SpiceCharDriver *s = chr->opaque;
> +
> +    spice_server_port_event(&s->sin, event);
> +#endif
> +}

No way.  You are passing an qemu-internal value (event) to an external
library here.  That is going to cause major grief in case the internal
change some day, so please don't.

I'd suggest to have something like this instead:

    switch (event) {
    case OPEN:
        spice_server_port_open()
        break;
    [ ... ]
    }

cheers,
  Gerd

PS: Small historical lesson:  spice-server 0.4.x (IIRC) was full of
    these, which was a major blocker of the upstream merge of spice
    support.  spice-server 0.6.x got a radically different library
    interface to fix that.  A few places escaped review, so we still
    have this in a few minor places, mouse button numbering for
    example.  Luckily this is a place where changes are unlikely.
    But please don't let us add new ones.
On Fri, Jun 21, 2013 at 9:35 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > +static void spice_chr_fe_event(struct CharDriverState *chr, int event)
> > +{
> > +#if SPICE_SERVER_VERSION >= 0x000c02
> > +    SpiceCharDriver *s = chr->opaque;
> > +
> > +    spice_server_port_event(&s->sin, event);
> > +#endif
> > +}
>
> No way.  You are passing an qemu-internal value (event) to an external
> library here.  That is going to cause major grief in case the internal
> change some day, so please don't.
>
> I'd suggest to have something like this instead:
>
>     switch (event) {
>     case OPEN:
>         spice_server_port_open()
>         break;
>     [ ... ]
>     }
>

Oh yeah, agreed.

Since the Spice server API already has spice_server_port_event() and
events, I will change it to:

switch (event) {
case OPEN:
    spice_server_port_open(SPICE_PORT_EVENT_OPENED)



> cheers,
>   Gerd
>
> PS: Small historical lesson:  spice-server 0.4.x (IIRC) was full of
>     these, which was a major blocker of the upstream merge of spice
>     support.  spice-server 0.6.x got a radically different library
>     interface to fix that.  A few places escaped review, so we still
>     have this in a few minor places, mouse button numbering for
>     example.  Luckily this is a place where changes are unlikely.
>     But please don't let us add new ones.
>
>