>
> On Mon, 2018-01-15 at 09:10 +, 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
> > ---
> > 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;
> > }
> >
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
