[Spice-devel] [PATCH spice-server 3/8] stream-device: Implements properly device reset on open/close

[Christophe Fergeau]

On Sun, Feb 18, 2018 at 06:58:09PM +0000, Frediano Ziglio wrote:
> Due to the way Qemu handle the device we must consume all pending
> data inside the callback to read data from the device.

"handles the device, we must .."
"... the callback which reads data ..." ?

I would add "when an error occurs":
"Due to the way Qemu handles the device, when an error occurs we must
consume all pending data ..."

> We need to consume all data from previous device dialog to avoid
> that next device usage is still seeing old data.

Maybe "If we don't flush this data, the next time we try to read from
the device, we'll be getting stale data".


> This as Qemu return 0 if you call SpiceCharDeviceInterface::read
> outside this callback (which is called by Qemu using
> spice_server_char_device_wakeup).

I suggested previously
"This needs to be done within this callback, as QEMU returns 0 if you
call SpiceCharDeviceInterface::read() outside of it"? "QEMU invokes this
callback through a call to spice_server_char_device_wakeup"

> On the test now we must test that we receive an error from the device.
> This as previously we checked that last part of the data was not read
> but now potentially all data are readed so we need another way to check
> the device detected the error.

I would remove the "This as"
"Previously we checked that the last part of the data was not read. Now
potentially all data are read, so we need another way to check the
device detected the error".

> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/stream-device.c            |  15 +++++-
>  server/tests/test-stream-device.c | 108 +++++++++++++++++++++++++-------------
>  2 files changed, 85 insertions(+), 38 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index cbd34463..f22946fb 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -84,11 +84,22 @@ stream_device_partial_read(StreamDevice *dev, SpiceCharDeviceInstance *sin)
>      int n;
>      bool handled = false;
>  
> -    if (dev->has_error || dev->flow_stopped || !dev->stream_channel) {
> +    sif = spice_char_device_get_interface(sin);
> +
> +    // in order to get in sync every time we open the device we need
> +    // to discard data here. This as Qemu keeps a buffer of data which
> +    // is used only during spice_server_char_device_wakeup from Qemu

Same comment about removing "This as"

> +    if (G_UNLIKELY(dev->has_error)) {
> +        uint8_t buf[16 * 1024];
> +        while (sif->read(sin, buf, sizeof(buf)) > 0) {
> +            continue;
> +        }
>          return false;

I was wondering if this flushing of the data could be done in
handle_invalid_msg() instead, when dev->has_error is set. It would make
more sense there when reading the code.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180223/f4d55fb0/attachment.sig>