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@xxxxxxxxxx>
> > > ---
> > > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel