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

[Marc-André Lureau]

On Fri, Jun 21, 2013 at 9:35 AM, Gerd Hoffmann <kraxel at 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.
>
>


-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20130621/c033bf5b/attachment.html>