Re: [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 <address@hidden> 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