Re: [PATCH spice-server] StreamDevice: move header reset to calling function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]