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

[Frediano Ziglio]

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

I think another solution instead of calling red_char_device_wakeup inside
stream_device_read_msg_from_dev would be to create a loop (maybe using a
wrapper function).

Frediano

> 
> 
> > 
> > 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;
> >  }
> >  
>