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

[Christophe Fergeau]

On Tue, Feb 27, 2018 at 06:20:32AM -0500, Frediano Ziglio wrote:
> > 
> > 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
> > 
> 
> Is not enough, guest can continue writing (for instance if need to write a long
> message) and we need to discard buffer. If we could stop Qemu getting data
> as soon as possible (see bug and workaround in following patches) and avoid
> queueing data this would be possible.

If I understand correctly your answer to the following patch, even after
setting the state to 0, errors will be returned to the user for
read/writes, but this code still needs to discard written data to
prevent losing sync?

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/20180305/133e395a/attachment.sig>