>
> Right now, each handle_msg_*() function is required to reset the header
> parsing state after it is done handling the message. Although the code
> duplication is pretty minimal (just resetting hdr_pos to 0), it seems a
> bit cleaner to have the calling function
> (stream_device_read_msg_from_dev()) handle this. Change the message
> handler functions to return a boolean value that signals whether they've
> fully handled the message or not. If the return value is true, the
> calling function will reset the message parsing state so that we're
> prepared to parse the next mesage.
> ---
>
> Proposed addition to Frediano's patch series. (Also somewhat related to
> Christophe D's question about resetting hdr_pos to 0 in patch 22)
>
Was thinking about doing something about it.
Looks good.
I was also thinking instead of passing a SpiceCharDeviceInstance
to pass a callback to read data and stop when message data is finished
but maybe is just overkilling.
Should I squash this change along the other patches (beside last hunk) ?
Frediano
> server/stream-device.c | 69
> +++++++++++++++++++++++++++-----------------------
> 1 file changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 3d33ca13d..a0383ae3e 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -61,7 +61,7 @@ static StreamDevice
> *stream_device_new(SpiceCharDeviceInstance *sin, RedsState *
>
> G_DEFINE_TYPE(StreamDevice, stream_device, RED_TYPE_CHAR_DEVICE)
>
> -typedef void StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> *sin);
> +typedef bool StreamMsgHandler(StreamDevice *dev, SpiceCharDeviceInstance
> *sin);
>
> static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_invalid,
> handle_msg_cursor_set, handle_msg_cursor_move;
> @@ -72,6 +72,7 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *si
> StreamDevice *dev = STREAM_DEVICE(self);
> SpiceCharDeviceInterface *sif;
> int n;
> + bool handled = false;
>
> if (dev->has_error || dev->flow_stopped || !dev->stream_channel) {
> return NULL;
> @@ -100,35 +101,41 @@ stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *si
> switch ((StreamMsgType) dev->hdr.type) {
> case STREAM_TYPE_FORMAT:
> if (dev->hdr.size != sizeof(StreamMsgFormat)) {
> - handle_msg_invalid(dev, sin);
> + handled = handle_msg_invalid(dev, sin);
> } else {
> - handle_msg_format(dev, sin);
> + handled = handle_msg_format(dev, sin);
> }
> break;
> case STREAM_TYPE_DATA:
> - handle_msg_data(dev, sin);
> + handled = handle_msg_data(dev, sin);
> break;
> case STREAM_TYPE_CURSOR_SET:
> - handle_msg_cursor_set(dev, sin);
> + handled = handle_msg_cursor_set(dev, sin);
> break;
> case STREAM_TYPE_CURSOR_MOVE:
> if (dev->hdr.size != sizeof(StreamMsgCursorMove)) {
> - handle_msg_invalid(dev, sin);
> + handled = handle_msg_invalid(dev, sin);
> } else {
> - handle_msg_cursor_move(dev, sin);
> + handled = handle_msg_cursor_move(dev, sin);
> }
> break;
> case STREAM_TYPE_CAPABILITIES:
> /* FIXME */
> default:
> - handle_msg_invalid(dev, sin);
> + handled = handle_msg_invalid(dev, sin);
> break;
> }
>
> + /* current message has been handled, so reset state and get ready to
> parse
> + * the next message */
> + if (handled) {
> + dev->hdr_pos = 0;
> + }
> +
> return NULL;
> }
>
> -static void
> +static bool
> handle_msg_invalid(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> static const char error_msg[] = "Wrong protocol";
> @@ -154,28 +161,29 @@ handle_msg_invalid(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
> red_char_device_write_buffer_add(char_dev, buf);
>
> dev->has_error = true;
> + return true;
> }
>
> -static void
> +static bool
> handle_msg_format(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> StreamMsgFormat fmt;
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> int n = sif->read(sin, (uint8_t *) &fmt, sizeof(fmt));
> if (n == 0) {
> - return;
> + return false;
> }
> if (n != sizeof(fmt)) {
> - handle_msg_invalid(dev, sin);
> - return;
> + return handle_msg_invalid(dev, sin);
> }
> fmt.width = GUINT32_FROM_LE(fmt.width);
> fmt.height = GUINT32_FROM_LE(fmt.height);
> stream_channel_change_format(dev->stream_channel, &fmt);
> - dev->hdr_pos = 0;
> +
> + return true;
> }
>
> -static void
> +static bool
> handle_msg_data(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> @@ -193,8 +201,10 @@ handle_msg_data(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
> dev->hdr.size -= n;
> }
> if (dev->hdr.size == 0) {
> - dev->hdr_pos = 0;
> + return true;
> }
> +
> + return false;
> }
>
> /*
> @@ -262,7 +272,7 @@ stream_msg_cursor_set_to_cursor_cmd(const
> StreamMsgCursorSet *msg, size_t msg_si
> return cmd;
> }
>
> -static void
> +static bool
> handle_msg_cursor_set(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> const unsigned int max_cursor_set_size =
> @@ -273,45 +283,41 @@ handle_msg_cursor_set(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
> // read part of the message till we get all
> if (!dev->msg_buf) {
> if (dev->hdr.size >= max_cursor_set_size || dev->hdr.size <
> sizeof(StreamMsgCursorSet)) {
> - handle_msg_invalid(dev, sin);
> - return;
> + return handle_msg_invalid(dev, sin);
> }
> dev->msg_buf = spice_malloc(dev->hdr.size);
> dev->msg_len = 0;
> }
> int n = sif->read(sin, (uint8_t *) dev->msg_buf + dev->msg_len,
> dev->hdr.size - dev->msg_len);
> if (n <= 0) {
> - return;
> + return false;
> }
> dev->msg_len += n;
> if (dev->msg_len != dev->hdr.size) {
> - return;
> + return false;
> }
>
> - // got the full message, prepare for future message
> - dev->hdr_pos = 0;
> -
> // trasform the message to a cursor command and process it
> RedCursorCmd *cmd = stream_msg_cursor_set_to_cursor_cmd(dev->msg_buf,
> dev->msg_len);
> if (!cmd) {
> - handle_msg_invalid(dev, sin);
> - return;
> + return handle_msg_invalid(dev, sin);
> }
> cursor_channel_process_cmd(dev->cursor_channel, cmd);
> +
> + return true;
> }
>
> -static void
> +static bool
> handle_msg_cursor_move(StreamDevice *dev, SpiceCharDeviceInstance *sin)
> {
> StreamMsgCursorMove move;
> SpiceCharDeviceInterface *sif = spice_char_device_get_interface(sin);
> int n = sif->read(sin, (uint8_t *) &move, sizeof(move));
> if (n == 0) {
> - return;
> + return false;
> }
> if (n != sizeof(move)) {
> - handle_msg_invalid(dev, sin);
> - return;
> + return handle_msg_invalid(dev, sin);
> }
> move.x = GINT32_FROM_LE(move.x);
> move.y = GINT32_FROM_LE(move.y);
> @@ -323,7 +329,7 @@ handle_msg_cursor_move(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>
> cursor_channel_process_cmd(dev->cursor_channel, cmd);
>
> - dev->hdr_pos = 0;
> + return true;
> }
>
> static void
> @@ -482,7 +488,6 @@ stream_device_port_event(RedCharDevice *char_dev, uint8_t
> event)
> if (device->opened) {
> allocate_channels(device);
> }
> - device->hdr_pos = 0;
> device->has_error = false;
> device->flow_stopped = false;
> red_char_device_reset(char_dev);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel