[Qemu-devel] [PATCH 1/3] spice-qemu-char: Generate chardev open/close events

[Qemu-devel] [PATCH 1/3] spice-qemu-char: Generate chardev open/close events

[Hans de Goede]

Define a state callback and make that generate chardev open/close events when
called by the spice-server.

Note that for all but the newest spice-server versions (which have a fix for
this) the code ignores these events for a spicevmc with a subtype of vdagent,
this subtype specific knowledge is undesirable, but unavoidable for now, see:
http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 spice-qemu-char.c |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/spice-qemu-char.c b/spice-qemu-char.c
index 95bf6b6..0a5059d 100644
--- a/spice-qemu-char.c
+++ b/spice-qemu-char.c
@@ -69,11 +69,50 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
     return bytes;
 }
 
+static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
+{
+    SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
+    int event;
+
+#if SPICE_SERVER_VERSION < 0x000901
+    /*
+     * spice-server calls the state callback for the agent channel when the
+     * spice client connects / disconnects. Given that not the client but
+     * the server is doing the parsing of the messages this is wrong as the
+     * server is still listening. Worse, this causes the parser in the server
+     * to go out of sync, so we ignore state calls for subtype vdagent
+     * spicevmc chardevs. For the full story see:
+     * http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
+     */
+    if (strcmp(sin->subtype, "vdagent") == 0) {
+        return;
+    }
+#endif
+
+    if ((scd->chr->opened && connected) ||
+        (!scd->chr->opened && !connected)) {
+        return;
+    }
+
+    if (connected) {
+        scd->chr->opened = 1;
+        event = CHR_EVENT_OPENED;
+    } else {
+        scd->chr->opened = 0;
+        event = CHR_EVENT_CLOSED;
+    }
+
+    if (scd->chr->chr_event) {
+        scd->chr->chr_event(scd->chr->handler_opaque, event);
+    }
+}
+
 static SpiceCharDeviceInterface vmc_interface = {
     .base.type          = SPICE_INTERFACE_CHAR_DEVICE,
     .base.description   = "spice virtual channel char device",
     .base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
     .base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
+    .state              = vmc_state,
     .write              = vmc_write,
     .read               = vmc_read,
 };
@@ -197,7 +236,12 @@ int qemu_chr_open_spice(QemuOpts *opts, CharDriverState **_chr)
     chr->chr_guest_open = spice_chr_guest_open;
     chr->chr_guest_close = spice_chr_guest_close;
 
-    qemu_chr_generic_open(chr);
+#if SPICE_SERVER_VERSION < 0x000901
+    /* See comment in vmc_state() */
+    if (strcmp(subtype, "vdagent") == 0) {
+        qemu_chr_generic_open(chr);
+    }
+#endif
 
     *_chr = chr;
     return 0;
-- 
1.7.5.1

[Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Hans de Goede]

To let the chardev now we're ready start receiving data. This is necessary
with the spicevmc chardev to get it registered with the spice-server.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index e212993..ec88c0b 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
 
     qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
                           usbredir_chardev_read, usbredir_chardev_event, dev);
+    /* Let the other side know we are ready */
+    qemu_chr_guest_open(dev->cs);
 
     return 0;
 }
@@ -830,6 +832,7 @@ static void usbredir_handle_destroy(USBDevice *udev)
 {
     USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
 
+    qemu_chr_guest_close(dev->cs);
     qemu_chr_close(dev->cs);
     /* Note must be done after qemu_chr_close, as that causes a close event */
     qemu_bh_delete(dev->open_close_bh);
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Anthony Liguori]

On 08/07/2011 08:21 AM, Hans de Goede wrote:
> To let the chardev now we're ready start receiving data. This is necessary
> with the spicevmc chardev to get it registered with the spice-server.
>
> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
> ---
>   usb-redir.c |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/usb-redir.c b/usb-redir.c
> index e212993..ec88c0b 100644
> --- a/usb-redir.c
> +++ b/usb-redir.c
> @@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
>
>       qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
>                             usbredir_chardev_read, usbredir_chardev_event, dev);
> +    /* Let the other side know we are ready */
> +    qemu_chr_guest_open(dev->cs);


You should do guest_open before adding handlers.

Regards,

Anthony Liguori

>
>       return 0;
>   }
> @@ -830,6 +832,7 @@ static void usbredir_handle_destroy(USBDevice *udev)
>   {
>       USBRedirDevice *dev = DO_UPCAST(USBRedirDevice, dev, udev);
>
> +    qemu_chr_guest_close(dev->cs);
>       qemu_chr_close(dev->cs);
>       /* Note must be done after qemu_chr_close, as that causes a close event */
>       qemu_bh_delete(dev->open_close_bh);

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Hans de Goede]

Hi,

On 08/07/2011 05:52 PM, Anthony Liguori wrote:
> On 08/07/2011 08:21 AM, Hans de Goede wrote:
>> To let the chardev now we're ready start receiving data. This is necessary
>> with the spicevmc chardev to get it registered with the spice-server.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>> usb-redir.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/usb-redir.c b/usb-redir.c
>> index e212993..ec88c0b 100644
>> --- a/usb-redir.c
>> +++ b/usb-redir.c
>> @@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
>>
>> qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
>> usbredir_chardev_read, usbredir_chardev_event, dev);
>> + /* Let the other side know we are ready */
>> + qemu_chr_guest_open(dev->cs);
>
>
> You should do guest_open before adding handlers.

Erm, no, guest_open may lead to a callback in the
chardev, to which it may respond by immediately queuing a few writes /
doing a read. To me it makes much more sense to actually call guest_open
when we are ready to receive data / to be read from, rather then to do
it before our handlers are hooked up and thus before we are ready.

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Anthony Liguori]

On 08/07/2011 12:41 PM, Hans de Goede wrote:
> Hi,
>
> On 08/07/2011 05:52 PM, Anthony Liguori wrote:
>> On 08/07/2011 08:21 AM, Hans de Goede wrote:
>>> To let the chardev now we're ready start receiving data. This is
>>> necessary
>>> with the spicevmc chardev to get it registered with the spice-server.
>>>
>>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>>> ---
>>> usb-redir.c | 3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/usb-redir.c b/usb-redir.c
>>> index e212993..ec88c0b 100644
>>> --- a/usb-redir.c
>>> +++ b/usb-redir.c
>>> @@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
>>>
>>> qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
>>> usbredir_chardev_read, usbredir_chardev_event, dev);
>>> + /* Let the other side know we are ready */
>>> + qemu_chr_guest_open(dev->cs);
>>
>>
>> You should do guest_open before adding handlers.
>
> Erm, no, guest_open may lead to a callback in the
> chardev, to which it may respond by immediately queuing a few writes /
> doing a read.

So after my char-flow changes, you won't be allowed to set handlers 
unless you've called open.

We want qemu_chr_guest_open() -> qemu_chr_fe_open() and for it to be 
analogous to a qemu_chr_be_open() which would be called immediately 
after accept() returned on a socket to signal that the backend is opened.

Because there's an intermediate queue, even if a write happens after 
open, no data will be lost.

So conceptionally, it makes sense to set handlers after open IMHO.

But most importantly to this series, no backend can possibly generate a 
write before you get to call add handlers so you've got nothing to worry 
about here (based on the code today).

Regards,

Anthony Liguori

  To me it makes much more sense to actually call guest_open
> when we are ready to receive data / to be read from, rather then to do
> it before our handlers are hooked up and thus before we are ready.
>
> Regards,
>
> Hans
>

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Hans de Goede]

Hi,

On 08/07/2011 11:30 PM, Anthony Liguori wrote:
> On 08/07/2011 12:41 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 08/07/2011 05:52 PM, Anthony Liguori wrote:
>>> On 08/07/2011 08:21 AM, Hans de Goede wrote:
>>>> To let the chardev now we're ready start receiving data. This is
>>>> necessary
>>>> with the spicevmc chardev to get it registered with the spice-server.
>>>>
>>>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>>>> ---
>>>> usb-redir.c | 3 +++
>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/usb-redir.c b/usb-redir.c
>>>> index e212993..ec88c0b 100644
>>>> --- a/usb-redir.c
>>>> +++ b/usb-redir.c
>>>> @@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
>>>>
>>>> qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
>>>> usbredir_chardev_read, usbredir_chardev_event, dev);
>>>> + /* Let the other side know we are ready */
>>>> + qemu_chr_guest_open(dev->cs);
>>>
>>>
>>> You should do guest_open before adding handlers.
>>
>> Erm, no, guest_open may lead to a callback in the
>> chardev, to which it may respond by immediately queuing a few writes /
>> doing a read.
>
> So after my char-flow changes, you won't be allowed to set handlers unless you've called open.
>

Why not do it the other way around? So don't allow open until the handlers are set. My reasoning
behind this is that eventually we will want to have a struct describing a pipe endpoint, which
will contain handlers (by then identical for both sides) and besides the struct a priv / user_data
pointer which will get passed by the handlers when called.

Then we will have a chardev_create or pipe_create call which will take a struct + user data ptr
for both ends (so twice). This matches what currently our set handlers call does. But I would
expect the open to come after the creation of the pipe.

At least to me it is much more logical to first set the handlers (which are really part
of object creation) and then later do the open, this matches the common programming
paradigm of having an init/create function and an open function.

Also forcing the set handlers after the open does not work well with virtio_console, as these
are not open until the port inside the guest is opened. So then it would need to delay its
set handlers till the first open, and what should it do at close, do a set handlers NULL
before doing the actual close ??

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Anthony Liguori]

On 08/08/2011 03:01 AM, Hans de Goede wrote:
> Hi,
>
> On 08/07/2011 11:30 PM, Anthony Liguori wrote:
>> On 08/07/2011 12:41 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 08/07/2011 05:52 PM, Anthony Liguori wrote:
>>>> On 08/07/2011 08:21 AM, Hans de Goede wrote:
>>>>> To let the chardev now we're ready start receiving data. This is
>>>>> necessary
>>>>> with the spicevmc chardev to get it registered with the spice-server.
>>>>>
>>>>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>>>>> ---
>>>>> usb-redir.c | 3 +++
>>>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/usb-redir.c b/usb-redir.c
>>>>> index e212993..ec88c0b 100644
>>>>> --- a/usb-redir.c
>>>>> +++ b/usb-redir.c
>>>>> @@ -809,6 +809,8 @@ static int usbredir_initfn(USBDevice *udev)
>>>>>
>>>>> qemu_chr_add_handlers(dev->cs, usbredir_chardev_can_read,
>>>>> usbredir_chardev_read, usbredir_chardev_event, dev);
>>>>> + /* Let the other side know we are ready */
>>>>> + qemu_chr_guest_open(dev->cs);
>>>>
>>>>
>>>> You should do guest_open before adding handlers.
>>>
>>> Erm, no, guest_open may lead to a callback in the
>>> chardev, to which it may respond by immediately queuing a few writes /
>>> doing a read.
>>
>> So after my char-flow changes, you won't be allowed to set handlers
>> unless you've called open.
>>
>
> Why not do it the other way around? So don't allow open until the
> handlers are set. My reasoning
> behind this is that eventually we will want to have a struct describing
> a pipe endpoint, which
> will contain handlers (by then identical for both sides) and besides the
> struct a priv / user_data
> pointer which will get passed by the handlers when called.
>
> Then we will have a chardev_create or pipe_create call which will take a
> struct + user data ptr
> for both ends (so twice). This matches what currently our set handlers
> call does. But I would
> expect the open to come after the creation of the pipe.

BTW, I'm 90% of the way there in my queue:

http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/char-flow

My plan is to have a CharPipe structure that has two CharDriverStates 
embedded in it.  The backend/frontends need to attach themselves to the 
CharDriverState.  I see that as open().

>
> At least to me it is much more logical to first set the handlers (which
> are really part
> of object creation) and then later do the open, this matches the common
> programming
> paradigm of having an init/create function and an open function.

But you need to change the handlers all of the time to implement flow 
control.  Today we overload the setting of handlers to have semantic 
meaning beyond setting the callbacks for various events.

The paradigm I think of is open()'ing a file, and then select()'ing on a 
file descriptor.

> Also forcing the set handlers after the open does not work well with
> virtio_console, as these
> are not open until the port inside the guest is opened. So then it would
> need to delay its
> set handlers till the first open,

Right, what's the problem with this?

  and what should it do at close, do a
> set handlers NULL
> before doing the actual close ??

No, close will automatically remove any added handlers.

Regards,

Anthony Liguori

>
> Regards,
>
> Hans
>

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Hans de Goede]

Hi,

On 08/08/2011 02:52 PM, Anthony Liguori wrote:
> On 08/08/2011 03:01 AM, Hans de Goede wrote:

<snip>

>>> So after my char-flow changes, you won't be allowed to set handlers
>>> unless you've called open.
>>>
>>
>> Why not do it the other way around? So don't allow open until the
>> handlers are set. My reasoning
>> behind this is that eventually we will want to have a struct describing
>> a pipe endpoint, which
>> will contain handlers (by then identical for both sides) and besides the
>> struct a priv / user_data
>> pointer which will get passed by the handlers when called.
>>
>> Then we will have a chardev_create or pipe_create call which will take a
>> struct + user data ptr
>> for both ends (so twice). This matches what currently our set handlers
>> call does. But I would
>> expect the open to come after the creation of the pipe.
>
> BTW, I'm 90% of the way there in my queue:
>
> http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/char-flow
>
> My plan is to have a CharPipe structure that has two CharDriverStates embedded in it. The backend/frontends need to attach themselves to the CharDriverState. I see that as open().
>

So the attaching will cause the other end to see an open() (if the
other end is already attached) ? Or will their still be a separate
send open event call? And should that call be made before or after
the attach?

Regards,

Hans

Re: [Qemu-devel] [PATCH 2/3] usb-redir: Call qemu_chr_guest_open/close

[Anthony Liguori]

On 08/08/2011 08:03 AM, Hans de Goede wrote:
> Hi,
>
> On 08/08/2011 02:52 PM, Anthony Liguori wrote:
>> On 08/08/2011 03:01 AM, Hans de Goede wrote:
>
> <snip>
>
>>>> So after my char-flow changes, you won't be allowed to set handlers
>>>> unless you've called open.
>>>>
>>>
>>> Why not do it the other way around? So don't allow open until the
>>> handlers are set. My reasoning
>>> behind this is that eventually we will want to have a struct describing
>>> a pipe endpoint, which
>>> will contain handlers (by then identical for both sides) and besides the
>>> struct a priv / user_data
>>> pointer which will get passed by the handlers when called.
>>>
>>> Then we will have a chardev_create or pipe_create call which will take a
>>> struct + user data ptr
>>> for both ends (so twice). This matches what currently our set handlers
>>> call does. But I would
>>> expect the open to come after the creation of the pipe.
>>
>> BTW, I'm 90% of the way there in my queue:
>>
>> http://repo.or.cz/w/qemu/aliguori.git/shortlog/refs/heads/char-flow
>>
>> My plan is to have a CharPipe structure that has two CharDriverStates
>> embedded in it. The backend/frontends need to attach themselves to the
>> CharDriverState. I see that as open().
>>
>
> So the attaching will cause the other end to see an open() (if the
> other end is already attached) ? Or will their still be a separate
> send open event call? And should that call be made before or after
> the attach?

Doing an open() will result in an open event being generated.  Neither 
side should ever be directly involved in sending open/close events.

Regards,

Anthony Liguori

>
> Regards,
>
> Hans
>

[Qemu-devel] [PATCH 3/3] usb-redir: Device disconnect + re-connect robustness fixes

[Hans de Goede]

These fixes mainly target the other side sending some (error status)
packets after a disconnect packet. In some cases these would get queued
up and then reported to the controller when a new device gets connected.

* Fully reset device state on disconnect
* Don't allow a connect message when already connected
* Ignore iso and interrupt status messages when disconnected

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 usb-redir.c |   22 +++++++++++++++++++++-
 1 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/usb-redir.c b/usb-redir.c
index ec88c0b..5d9483d 100644
--- a/usb-redir.c
+++ b/usb-redir.c
@@ -874,6 +874,11 @@ static void usbredir_device_connect(void *priv,
 {
     USBRedirDevice *dev = priv;
 
+    if (qemu_timer_pending(dev->attach_timer) || dev->dev.attached) {
+        ERROR("Received device connect while already connected\n");
+        return;
+    }
+
     switch (device_connect->speed) {
     case usb_redir_speed_low:
         DPRINTF("attaching low speed device\n");
@@ -902,19 +907,26 @@ static void usbredir_device_connect(void *priv,
 static void usbredir_device_disconnect(void *priv)
 {
     USBRedirDevice *dev = priv;
+    int i;
 
     /* Stop any pending attaches */
     qemu_del_timer(dev->attach_timer);
 
     if (dev->dev.attached) {
         usb_device_detach(&dev->dev);
-        usbredir_cleanup_device_queues(dev);
         /*
          * Delay next usb device attach to give the guest a chance to see
          * see the detach / attach in case of quick close / open succession
          */
         dev->next_attach_time = qemu_get_clock_ms(vm_clock) + 200;
     }
+
+    /* Reset state so that the next dev connected starts with a clean slate */
+    usbredir_cleanup_device_queues(dev);
+    memset(dev->endpoint, 0, sizeof(dev->endpoint));
+    for (i = 0; i < MAX_ENDPOINTS; i++) {
+        QTAILQ_INIT(&dev->endpoint[i].bufpq);
+    }
 }
 
 static void usbredir_interface_info(void *priv,
@@ -1006,6 +1018,10 @@ static void usbredir_iso_stream_status(void *priv, uint32_t id,
     DPRINTF("iso status %d ep %02X id %u\n", iso_stream_status->status,
             ep, id);
 
+    if (!dev->dev.attached) {
+        return;
+    }
+
     dev->endpoint[EP2I(ep)].iso_error = iso_stream_status->status;
     if (iso_stream_status->status == usb_redir_stall) {
         DPRINTF("iso stream stopped by peer ep %02X\n", ep);
@@ -1023,6 +1039,10 @@ static void usbredir_interrupt_receiving_status(void *priv, uint32_t id,
     DPRINTF("interrupt recv status %d ep %02X id %u\n",
             interrupt_receiving_status->status, ep, id);
 
+    if (!dev->dev.attached) {
+        return;
+    }
+
     dev->endpoint[EP2I(ep)].interrupt_error =
         interrupt_receiving_status->status;
     if (interrupt_receiving_status->status == usb_redir_stall) {
-- 
1.7.5.1

Re: [Qemu-devel] [PATCH 1/3] spice-qemu-char: Generate chardev open/close events

[Anthony Liguori]

On 08/07/2011 08:21 AM, Hans de Goede wrote:
> Define a state callback and make that generate chardev open/close events when
> called by the spice-server.
>
> Note that for all but the newest spice-server versions (which have a fix for
> this) the code ignores these events for a spicevmc with a subtype of vdagent,
> this subtype specific knowledge is undesirable, but unavoidable for now, see:
> http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
>
> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
> ---
>   spice-qemu-char.c |   46 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 files changed, 45 insertions(+), 1 deletions(-)
>
> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
> index 95bf6b6..0a5059d 100644
> --- a/spice-qemu-char.c
> +++ b/spice-qemu-char.c
> @@ -69,11 +69,50 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
>       return bytes;
>   }
>
> +static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
> +{
> +    SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
> +    int event;
> +
> +#if SPICE_SERVER_VERSION<  0x000901
> +    /*
> +     * spice-server calls the state callback for the agent channel when the
> +     * spice client connects / disconnects. Given that not the client but
> +     * the server is doing the parsing of the messages this is wrong as the
> +     * server is still listening. Worse, this causes the parser in the server
> +     * to go out of sync, so we ignore state calls for subtype vdagent
> +     * spicevmc chardevs. For the full story see:
> +     * http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
> +     */
> +    if (strcmp(sin->subtype, "vdagent") == 0) {
> +        return;
> +    }
> +#endif
> +
> +    if ((scd->chr->opened&&  connected) ||
> +        (!scd->chr->opened&&  !connected)) {
> +        return;
> +    }
> +
> +    if (connected) {
> +        scd->chr->opened = 1;
> +        event = CHR_EVENT_OPENED;
> +    } else {
> +        scd->chr->opened = 0;
> +        event = CHR_EVENT_CLOSED;
> +    }
> +
> +    if (scd->chr->chr_event) {
> +        scd->chr->chr_event(scd->chr->handler_opaque, event);
> +    }

You should use qemu_chr_event and then this whole block of code 
disappears since it already manages the opened flag.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/3] spice-qemu-char: Generate chardev open/close events

[Hans de Goede]

Hi,

On 08/07/2011 05:52 PM, Anthony Liguori wrote:
> On 08/07/2011 08:21 AM, Hans de Goede wrote:
>> Define a state callback and make that generate chardev open/close events when
>> called by the spice-server.
>>
>> Note that for all but the newest spice-server versions (which have a fix for
>> this) the code ignores these events for a spicevmc with a subtype of vdagent,
>> this subtype specific knowledge is undesirable, but unavoidable for now, see:
>> http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>> spice-qemu-char.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 files changed, 45 insertions(+), 1 deletions(-)
>>
>> diff --git a/spice-qemu-char.c b/spice-qemu-char.c
>> index 95bf6b6..0a5059d 100644
>> --- a/spice-qemu-char.c
>> +++ b/spice-qemu-char.c
>> @@ -69,11 +69,50 @@ static int vmc_read(SpiceCharDeviceInstance *sin, uint8_t *buf, int len)
>> return bytes;
>> }
>>
>> +static void vmc_state(SpiceCharDeviceInstance *sin, int connected)
>> +{
>> + SpiceCharDriver *scd = container_of(sin, SpiceCharDriver, sin);
>> + int event;
>> +
>> +#if SPICE_SERVER_VERSION< 0x000901
>> + /*
>> + * spice-server calls the state callback for the agent channel when the
>> + * spice client connects / disconnects. Given that not the client but
>> + * the server is doing the parsing of the messages this is wrong as the
>> + * server is still listening. Worse, this causes the parser in the server
>> + * to go out of sync, so we ignore state calls for subtype vdagent
>> + * spicevmc chardevs. For the full story see:
>> + * http://lists.freedesktop.org/archives/spice-devel/2011-July/004837.html
>> + */
>> + if (strcmp(sin->subtype, "vdagent") == 0) {
>> + return;
>> + }
>> +#endif
>> +
>> + if ((scd->chr->opened&& connected) ||
>> + (!scd->chr->opened&& !connected)) {
>> + return;
>> + }
>> +
>> + if (connected) {
>> + scd->chr->opened = 1;
>> + event = CHR_EVENT_OPENED;
>> + } else {
>> + scd->chr->opened = 0;
>> + event = CHR_EVENT_CLOSED;
>> + }
>> +
>> + if (scd->chr->chr_event) {
>> + scd->chr->chr_event(scd->chr->handler_opaque, event);
>> + }
>
> You should use qemu_chr_event and then this whole block of code disappears since it already manages the opened flag.

Right, good one,

Regards,

Hans