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

[Jonathon Jongsma]

On Mon, 2018-01-15 at 09:10 +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.

So I think that this commit message is a little bit misleading. The
"stall" is caused because it seems that we're not really implementing
the RedCharDevice class as expected. 

stream_device_read_msg_from_dev() is the vfunc implementation of
RedCharDevice::read_one_msg_from_device(). This function expects the
implementation to read to the end of a single message and then return a
RedPipeItem* to be sent out to clients. If a non-NULL RedPipeItem* is
returned, red_char_device_read_from_device() sends that pipe item to
all clients and then continues looping and tries to read another
message from the device. However, if read_one_msg_from_device() returns
a NULL RedPipeItem*, it apparently assumes that there was not enough
data for a full message and there is nothing else to read, so it breaks
out of the loop. 

Our StreamDevice vfunc always returns NULL from
read_one_msg_from_device() because we handle the messages directly
rather then returning RedPipeItems to be sent on to a client. But this
means that RedCharDevice apparently thinks that we were not able to
read a full message from the device so it stops its loop. 

So calling red_char_device_wakeup() here seems a bit like a hack. It
ends up recursively calling red_char_device_read_from_device(). Reading
through some of the other RedCharDevice implementations, it seems like
we've solved this issue in other ways already. For instance
vdi_port_read_one_msg_from_device() reads a full message and then
determines whether it should be sent to the client or not. If it
should, it returns a RedPipeItem. But if it shouldn't be sent to the
client, it loops around and tries to read the next message. In other
words, this function doesn't necessarily just read one message. It
reads as many messages as necessary until it comes to a message that
should be sent to the client (or until it runs out of data to read).
But this behavior sort of duplicates the logic in the
red_char_device_read_from_device() function that calls this function. 

Maybe a better way to handle this is to change the
RedCharDevice::read_one_msg_from_device() vfunc to allow us to handle
this use case.

For instance, instead of:

    RedPipeItem* (*read_one_msg_from_device)(RedCharDevice *self,
                                             SpiceCharDeviceInstance
*sin);

It could be something like:

    bool (*read_one_msg_from_device)(RedCharDevice *self,
                                     SpiceCharDeviceInstance *sin,
                                     RedPipeItem **out_item);

This way we could handle 3 different scenarios:
 - return true and set *out_item to a new pipe item that should be sent
to the client
 - return true and leave out_item NULL to indicate that we read a
message but there is nothing to be sent to the client.
 - return false to indicate that there was not enough data available to
read a full message




> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/stream-device.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 4eaa959b..897fc665 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -123,6 +123,15 @@ 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
> +        red_char_device_wakeup(self);
> +    }
> +
>      return NULL;
>  }
>