> > 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 >
extended the comment. Basically msg_buf code handling could be used for different messages. > > document limit on cursor. > > or here? > Declared some constant in protocol and added some comments there. > > use finalize instead of dispose for message. > > or here > dispose is supposed to avoid some recursion on GObject, memory should be released on finalize. Fixed. > > reuse code to close and destroy channels > > This is related to https://lists.freedesktop.org/archives/spice-devel/2017-August/039339.html > > Signed-off-by: Frediano Ziglio <fzig...@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? > There's currently no support for position, maybe I should add some TODO (like line below) 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 > wrote a function to compute bits and add some comments > > + 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) ? > done using new declarations from protocol header > > + 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; > > + } this code would be smaller with patch mentioned above. > > + free(device->msg_buf); > > + device->msg_buf = 0; Move these 2 lines in a new finalize > > } > > > > static void > > > Looks OK in general. > Frediano _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
