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

[Frediano Ziglio]

> 
> 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.

Frediano