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

[Christophe Fergeau]

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


> 
> 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 :(

Patch looks good otherwise,

Acked-by: Christophe Fergeau <cfergeau at redhat.com>

Christophe

> +    char_device_enable(char_dev);
>  }
>  
>  static void
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- 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/f40356b7/attachment.sig>