[Spice-devel] [RFC PATCH spice-server v2 18/19] WIP stream-device: handle cursor from device

[Jonathon Jongsma]

On Wed, 2017-06-14 at 16:40 +0100, Frediano Ziglio wrote:
> TODO: reuse message code (limit messages based on type ??)

I don't quite understand what you mean above

> document limit on cursor.

or here?

> use finalize instead of dispose for message.

or here

> reuse code to close and destroy channels
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  server/stream-device.c | 117
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/server/stream-device.c b/server/stream-device.c
> index 2b1e2f2..2513b8c 100644
> --- a/server/stream-device.c
> +++ b/server/stream-device.c
> @@ -23,6 +23,7 @@
>  
>  #include "char-device.h"
>  #include "stream-channel.h"
> +#include "cursor-channel.h"
>  #include "reds.h"
>  
>  #define TYPE_STREAM_DEVICE stream_device_get_type()
> @@ -46,6 +47,9 @@ struct StreamDevice {
>      bool opened;
>      bool flow_stopped;
>      StreamChannel *channel;
> +    CursorChannel *cursor_channel;
> +    void *msg_buf;
> +    uint32_t msg_len;
>  };
>  
>  struct StreamDeviceClass {
> @@ -59,7 +63,8 @@ G_DEFINE_TYPE(StreamDevice, stream_device,
> RED_TYPE_CHAR_DEVICE)
>  
>  typedef void StreamMsgHandler(StreamDevice *dev,
> SpiceCharDeviceInstance *sin);
>  
> -static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_invalid;
> +static StreamMsgHandler handle_msg_format, handle_msg_data,
> handle_msg_invalid,
> +    handle_msg_cursor_set;
>  
>  static RedPipeItem *
>  stream_device_read_msg_from_dev(RedCharDevice *self,
> SpiceCharDeviceInstance *sin)
> @@ -76,6 +81,11 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>  
>      /* read header */
>      while (dev->hdr_pos < sizeof(dev->hdr)) {
> +        if (dev->msg_buf) {
> +            free(dev->msg_buf);
> +            dev->msg_buf = NULL;
> +            dev->msg_len = 0;
> +        }
>          n = sif->read(sin, (uint8_t *) &dev->hdr, sizeof(dev->hdr) -
> dev->hdr_pos);
>          if (n <= 0) {
>              return NULL;
> @@ -98,6 +108,9 @@ stream_device_read_msg_from_dev(RedCharDevice
> *self, SpiceCharDeviceInstance *si
>      case STREAM_TYPE_DATA:
>          handle_msg_data(dev, sin);
>          break;
> +    case STREAM_TYPE_CURSOR_SET:
> +        handle_msg_cursor_set(dev, sin);
> +        break;
>      case STREAM_TYPE_CAPABILITIES:
>          /* FIXME */
>      default:
> @@ -177,6 +190,87 @@ handle_msg_data(StreamDevice *dev,
> SpiceCharDeviceInstance *sin)
>      }
>  }
>  
> +static RedCursorCmd *
> +stream_msg_cursor_set_to_cursor_cmd(const StreamMsgCursorSet *msg,
> size_t msg_size)
> +{
> +    const QXLCursorHeader* cursor_hdr = &msg->cursor_header;
> +
> +    RedCursorCmd *cmd = spice_new0(RedCursorCmd, 1);
> +    cmd->type = QXL_CURSOR_SET;
> +    cmd->u.set.position.x = 0;
> +    cmd->u.set.position.y = 0;

Why is the position hard-coded here?

> +    cmd->u.set.visible = 1; // TODO
> +    SpiceCursor *cursor = &cmd->u.set.shape;
> +    cursor->header.unique = GUINT64_FROM_LE(cursor_hdr->unique);
> +    cursor->header.type = GUINT16_FROM_LE(cursor_hdr->type);
> +    cursor->header.width = GUINT16_FROM_LE(cursor_hdr->width);
> +    cursor->header.height = GUINT16_FROM_LE(cursor_hdr->height);
> +    cursor->header.hot_spot_x = GUINT16_FROM_LE(cursor_hdr-
> >hot_spot_x);
> +    cursor->header.hot_spot_y = GUINT16_FROM_LE(cursor_hdr-
> >hot_spot_y);
> +    if (cursor->header.width > 1024 || cursor->header.height > 1024)
> {
> +        free(cmd);
> +        return NULL;
> +    }
> +    size_t size_required = cursor->header.width * cursor-
> >header.height;
> +    switch (cursor->header.type) {
> +    case SPICE_CURSOR_TYPE_ALPHA:
> +        size_required *= 32;
> +        break;
> +    case SPICE_CURSOR_TYPE_COLOR24:
> +        size_required *= 25;
> +        break;
> +    case SPICE_CURSOR_TYPE_COLOR32:
> +        size_required *= 33;
> +        break;
> +    default:
> +        free(cmd);
> +        return NULL;
> +    }
> +    size_required = (size_required + 7) / 8;

I think the above line deserves a short comment

> +    if (msg_size < sizeof(StreamMsgCursorSet) + size_required) {
> +        free(cmd);
> +        return NULL;
> +    }
> +    cursor->data_size = size_required;
> +    cursor->data = spice_memdup(msg->data, size_required);
> +    return cmd;
> +}
> +
> +static void
> +handle_msg_cursor_set(StreamDevice *dev, SpiceCharDeviceInstance
> *sin)
> +{
> +    SpiceCharDeviceInterface *sif =
> spice_char_device_get_interface(sin);
> +
> +    // read part of the message till we get all
> +    if (!dev->msg_buf) {
> +        if (dev->hdr.size >= 1024 * 1024 * 5 || dev->hdr.size <
> sizeof(StreamMsgCursorSet)) {

#define MAX_CURSOR_SIZE (1024 * 1024 * 5) ?

> +            handle_msg_invalid(dev, sin);
> +            return;
> +        }
> +        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;
> +    }
> +    dev->msg_len += n;
> +    if (dev->msg_len != dev->hdr.size) {
> +        return;
> +    }
> +
> +    // 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;
> +    }
> +    cursor_channel_process_cmd(dev->cursor_channel, cmd);
> +}
> +
>  static void
>  stream_device_send_msg_to_client(RedCharDevice *self, RedPipeItem
> *msg, RedClient *client)
>  {
> @@ -253,11 +347,19 @@ RedCharDevice *
>  stream_device_connect(RedsState *reds, SpiceCharDeviceInstance *sin)
>  {
>      SpiceCharDeviceInterface *sif;
> +    ClientCbs client_cbs = { NULL, };
>  
>      StreamChannel *channel = stream_channel_new(reds);
>  
> +    CursorChannel *cursor_channel = cursor_channel_new(reds, NULL,
> reds_get_core_interface(reds));
> +    client_cbs.connect = (channel_client_connect_proc)
> cursor_channel_connect;
> +    client_cbs.migrate = cursor_channel_client_migrate;
> +    red_channel_register_client_cbs(RED_CHANNEL(cursor_channel),
> &client_cbs, NULL);
> +    reds_register_channel(reds, RED_CHANNEL(cursor_channel));
> +
>      StreamDevice *dev = stream_device_new(sin, reds);
>      dev->channel = channel;
> +    dev->cursor_channel = cursor_channel;
>      stream_channel_register_start_cb(channel,
> stream_device_stream_start, dev);
>      stream_channel_register_queue_stat_cb(channel,
> stream_device_stream_queue_stat, dev);
>  
> @@ -285,6 +387,19 @@ stream_device_dispose(GObject *object)
>          red_channel_destroy(red_channel);
>          device->channel = NULL;
>      }
> +    if (device->cursor_channel) {
> +        RedChannel *red_channel = RED_CHANNEL(device-
> >cursor_channel);
> +        RedsState *reds = red_channel_get_server(red_channel);
> +
> +        // prevent future connection
> +        reds_unregister_channel(reds, red_channel);
> +
> +        // close all current connections and drop the reference
> +        red_channel_destroy(red_channel);
> +        device->cursor_channel = NULL;
> +    }
> +    free(device->msg_buf);
> +    device->msg_buf = 0;
>  }
>  
>  static void


Looks OK in general.