[Spice-devel] [RFC PATCH spice-server v2 18/19] WIP stream-device: handle cursor from device
Frediano Ziglio
fziglio at redhat.com
Wed Aug 23 09:01:38 UTC 2017
>
> 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 <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?
>
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
More information about the Spice-devel
mailing list