[Spice-devel] [PATCH spice-server 4/8] stream-device: Disable guest device on errors

[Frediano Ziglio]

> 
> On Sun, Feb 18, 2018 at 06:58:10PM +0000, Frediano Ziglio wrote:
> > To communicate the error and avoiding to have to read any data the
> > guest want to write disable the device.
> 
> I am having troubles parsing this. Is this missing a comma before
> "disable"?
> 
> "To communicate the error and avoiding to have to read any data the
> guest want to write, disable the device.". If so, I'd reformulate it,
> "Once the device is an error state, we don't want the guest to keep
> reading/writing to it, especially as this could put the device in an
> inconsistent state. This commit closes the device when an error occurs
> to prevent further unintended use of the device by the guest."
> 

Not only this. From the guest point of view after calling state(0) from
the server the guest will receive error reading/writing to the device.
If you discard data and the guest is ignoring possible errors from the
device you are basically wasting CPU queuing/dequeuing data from the
device (not a big deal like a DoS, guest can waste its CPU time as it
wants). If we don't discard data potentially the write will stop and
the program using the device will just go to sleep (blocking write).
If the server set the state to 0 is much more likely that the guest
program will detect the error.

> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/stream-device.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/server/stream-device.c b/server/stream-device.c
> > index f22946fb..688a4e07 100644
> > --- a/server/stream-device.c
> > +++ b/server/stream-device.c
> > @@ -94,6 +94,7 @@ stream_device_partial_read(StreamDevice *dev,
> > SpiceCharDeviceInstance *sin)
> >          while (sif->read(sin, buf, sizeof(buf)) > 0) {
> >              continue;
> >          }
> > +        sif->state(sin, 0);
> >          return false;
> >      }
> >  
> > @@ -560,6 +561,19 @@ reset_channels(StreamDevice *dev)
> >      }
> >  }
> >  
> > +static void
> > +char_device_enable(RedCharDevice *char_dev)
> > +{
> > +    SpiceCharDeviceInstance *sin = NULL;
> > +    g_object_get(char_dev, "sin", &sin, NULL);
> > +    spice_assert(sin != NULL);
> > +
> > +    SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> > +    if (sif->state) {
> > +        sif->state(sin, 1);
> > +    }
> > +}
> > +
> >  static void
> >  stream_device_port_event(RedCharDevice *char_dev, uint8_t event)
> >  {
> > @@ -580,6 +594,12 @@ stream_device_port_event(RedCharDevice *char_dev,
> > uint8_t event)
> >      dev->flow_stopped = false;
> >      red_char_device_reset(char_dev);
> >      reset_channels(dev);
> > +
> > +    // enable the device again. We try to enable it even on close as
> > +    // this could prevent a possible race condition where we open the
> > +    // device from the guest and it take some time to enable causing
> > +    // temporary writing issues.
> 
> I am not sure I understand the race from that comment :(
> 

I didn't dig much on the guest side. What I see is that after the error
the guest program exit (as detecting write error), you launch it again
and first open fails while second succeeds. Setting the state to 1 (active)
on the close event allow the guest to successfully open the device again.

> Patch looks good otherwise,
> 
> Acked-by: Christophe Fergeau <cfergeau at redhat.com>
> 
> Christophe
> 
> > +    char_device_enable(char_dev);
> >  }
> >  
> >  static void

Frediano