| Message ID | 1371750371-18491-3-git-send-email-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. > >
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);
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(+)