[Spice-devel] [PATCH spice-server v5 1/8] stream-device: Avoid device to get stuck if multiple messages are batched

[Jonathon Jongsma]

On Tue, 2018-02-13 at 15:54 +0000, Frediano Ziglio wrote:
> If messages are sent together by the agent the device is reading
> only part of the data. This cause Qemu to not poll for new data and
> stream_device_read_msg_from_dev won't be called again.
> This can cause a stall. To avoid this continue handling data
> after a full message was processed.
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/stream-device.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 4eaa959b..3a7cb306 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -71,16 +71,15 @@ static StreamMsgHandler handle_msg_format,
> handle_msg_data;
>  static bool handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin,
>                                 const char *error_msg)
> SPICE_GNUC_WARN_UNUSED_RESULT;
>  
> -static RedPipeItem *
> -stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *sin)
> +static bool
> +stream_device_partial_read(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>  {
> -    StreamDevice *dev = STREAM_DEVICE(self);
>      SpiceCharDeviceInterface *sif;
>      int n;
>      bool handled = false;
>  
>      if (dev->has_error || dev->flow_stopped || !dev->stream_channel) 
> {
> -        return NULL;
> +        return false;
>      }
>  
>      sif = spice_char_device_get_interface(sin);
> @@ -89,7 +88,7 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>      while (dev->hdr_pos < sizeof(dev->hdr)) {
>          n = sif->read(sin, (uint8_t *) &dev->hdr + dev->hdr_pos,
> sizeof(dev->hdr) - dev->hdr_pos);
>          if (n <= 0) {
> -            return NULL;
> +            return false;
>          }
>          dev->hdr_pos += n;
>          if (dev->hdr_pos >= sizeof(dev->hdr)) {
> @@ -123,6 +122,26 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>          dev->hdr_pos = 0;
>      }
>  
> +    if (handled || dev->has_error) {
> +        // Qemu put the device on blocking state if we don't read
> all data
> +        // so schedule another read.
> +        // We arrive here if we processed that entire message or we
> +        // got an error, try to read another message or discard the
> +        // wrong data
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static RedPipeItem *
> +stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *sin)
> +{
> +    StreamDevice *dev = STREAM_DEVICE(self);
> +
> +    while (stream_device_partial_read(dev, sin)) {
> +        continue;
> +    }
>      return NULL;
>  }
>  


Thanks. I prefer this approach instead of calling
red_char_device_wakeup(). The name stream_device_partial_read() is OK,
but it's not great. It is really reading up until the end of the next
message *or* until there is no more data to read. I thought about
stream_device_read_to_end_of_message(), but that's not very nice
either. I can't think of a great name.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>