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

[Gerd Hoffmann]

  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.