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