From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:54169)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1UpvsN-0004FR-SJ
	for qemu-devel@nongnu.org; Fri, 21 Jun 2013 03:35:20 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1UpvsN-0005zq-4k
	for qemu-devel@nongnu.org; Fri, 21 Jun 2013 03:35:19 -0400
Received: from mx1.redhat.com ([209.132.183.28]:54438)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <kraxel@redhat.com>) id 1UpvsM-0005zh-UQ
	for qemu-devel@nongnu.org; Fri, 21 Jun 2013 03:35:19 -0400
Message-ID: <51C40231.2070503@redhat.com>
Date: Fri, 21 Jun 2013 09:35:13 +0200
From: Gerd Hoffmann <kraxel@redhat.com>
MIME-Version: 1.0
References: <1371750371-18491-1-git-send-email-marcandre.lureau@redhat.com>
	<1371750371-18491-3-git-send-email-marcandre.lureau@redhat.com>
In-Reply-To: <1371750371-18491-3-git-send-email-marcandre.lureau@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 02/12] char: add qemu_chr_fe_event()
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= <marcandre.lureau@gmail.com>
Cc: spice-devel@lists.freedesktop.org, qemu-devel@nongnu.org, =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= <marcandre.lureau@redhat.com>

  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.