> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx> > > Also fix warning due to unexpected pipe item type > > The specific item type that was not being handled was > PIPE_ITEM_TYPE_INVAL_ONE (#102). This item type is used by the cursor > channel, but the analogous item for the display channel is > PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE. Use this value instead. > > The exact warning follows: > > (/usr/bin/qemu-kvm:24458): Spice-Warning **: > ../../server/dcc-send.c:2442:dcc_send_item: should not be reached > (/usr/bin/qemu-kvm:24458): Spice-CRITICAL **: > ../../server/dcc.c:1595:release_item_before_push: invalid item type > > Author: Marc-André Lureau <marcandre.lureau@xxxxxxxxx> > Date: Thu Feb 27 19:38:58 2014 +0200 > --- > server/cache_item.tmpl.c | 4 +- > server/cursor-channel.c | 219 > ++++++++++++++++++++++++++--------------------- > server/cursor-channel.h | 27 +++--- > server/red_dispatcher.c | 1 + > server/red_worker.c | 200 +++++++++++++++++++------------------------ > server/red_worker.h | 65 +++++--------- > 6 files changed, 255 insertions(+), 261 deletions(-) > > diff --git a/server/cache_item.tmpl.c b/server/cache_item.tmpl.c > index dc314c0..ad2b579 100644 > --- a/server/cache_item.tmpl.c > +++ b/server/cache_item.tmpl.c > @@ -21,6 +21,7 @@ > #define CACHE_HASH_KEY CURSOR_CACHE_HASH_KEY > #define CACHE_HASH_SIZE CURSOR_CACHE_HASH_SIZE > #define CACHE_INVAL_TYPE SPICE_MSG_CURSOR_INVAL_ONE > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_ONE I would prefer a PIPE_ITEM_TYPE_INVALID macro here. Is not clear which part of the patch is fixing the "unexpected pipe item type". > #define FUNC_NAME(name) red_cursor_cache_##name > #define VAR_NAME(name) cursor_cache_##name > #define CHANNEL CursorChannel > @@ -32,6 +33,7 @@ > #define CACHE_HASH_KEY PALETTE_CACHE_HASH_KEY > #define CACHE_HASH_SIZE PALETTE_CACHE_HASH_SIZE > #define CACHE_INVAL_TYPE SPICE_MSG_DISPLAY_INVAL_PALETTE > +#define PIPE_ITEM_TYPE PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE > #define FUNC_NAME(name) red_palette_cache_##name > #define VAR_NAME(name) palette_cache_##name > #define CHANNEL DisplayChannel > @@ -78,7 +80,7 @@ static void FUNC_NAME(remove)(CHANNELCLIENT > *channel_client, CacheItem *item) > channel_client->VAR_NAME(items)--; > channel_client->VAR_NAME(available) += item->size; > > - red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data, > PIPE_ITEM_TYPE_INVAL_ONE); > + red_channel_pipe_item_init(&channel->common.base, &item->u.pipe_data, > PIPE_ITEM_TYPE); > red_channel_client_pipe_add_tail(&channel_client->common.base, > &item->u.pipe_data); // for now > } > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c > index 7ba4a7d..adf7d78 100644 > --- a/server/cursor-channel.c > +++ b/server/cursor-channel.c > @@ -25,57 +25,64 @@ > #include "cache_item.tmpl.c" > #undef CLIENT_CURSOR_CACHE > > -static inline CursorItem *alloc_cursor_item(void) > +CursorItem *cursor_item_new(QXLInstance *qxl, RedCursorCmd *cmd, uint32_t > group_id) > { > CursorItem *cursor_item; > > + spice_return_val_if_fail(cmd != NULL, NULL); > + > cursor_item = g_slice_new0(CursorItem); > + cursor_item->qxl = qxl; > cursor_item->refs = 1; > + cursor_item->group_id = group_id; > + cursor_item->red_cursor = cmd; > > return cursor_item; > } > > -CursorItem *cursor_item_new(RedCursorCmd *cmd, uint32_t group_id) > +CursorItem *cursor_item_ref(CursorItem *item) > { > - CursorItem *cursor_item; > + spice_return_val_if_fail(item != NULL, NULL); > + spice_return_val_if_fail(item->refs > 0, NULL); This is detecting a memory error, perhaps a spice_error or a spice_critical should be better. > > - spice_return_val_if_fail(cmd != NULL, NULL); > - spice_warn_if(!(cursor_item = alloc_cursor_item())); > - > - cursor_item->group_id = group_id; > - cursor_item->red_cursor = cmd; > + item->refs++; > > - return cursor_item; > + return item; > } > > -void cursor_item_unref(QXLInstance *qxl, CursorItem *cursor) > +void cursor_item_unref(CursorItem *item) > { > - if (!--cursor->refs) { > - QXLReleaseInfoExt release_info_ext; > - RedCursorCmd *cursor_cmd; > - > - cursor_cmd = cursor->red_cursor; > - release_info_ext.group_id = cursor->group_id; > - release_info_ext.info = cursor_cmd->release_info; > - qxl->st->qif->release_resource(qxl, release_info_ext); > - red_put_cursor_cmd(cursor_cmd); > - free(cursor_cmd); > - > - g_slice_free(CursorItem, cursor); > - } > + QXLReleaseInfoExt release_info_ext; > + RedCursorCmd *cursor_cmd; > + > + spice_return_if_fail(item != NULL); > + > + if (--item->refs) > + return; > + > + cursor_cmd = item->red_cursor; > + release_info_ext.group_id = item->group_id; > + release_info_ext.info = cursor_cmd->release_info; > + item->qxl->st->qif->release_resource(item->qxl, release_info_ext); > + red_put_cursor_cmd(cursor_cmd); > + free(cursor_cmd); > + > + g_slice_free(CursorItem, item); > + > } > There are a lot of renames cursor_channel -> cursor and cursor -> item (here and in following hunks). Should we remove them ? Which naming should we use ? > static void cursor_set_item(CursorChannel *cursor, CursorItem *item) > { > if (cursor->item) > - cursor_item_unref(red_worker_get_qxl(cursor->common.worker), > cursor->item); > + cursor_item_unref(cursor->item); > > - if (item) > - item->refs++; > - > - cursor->item = item; > + cursor->item = item ? cursor_item_ref(item) : NULL; > } > qxl field in CursorItem is added mainly to be able to free it without passing a QXLInstance object which can be computed quite easily from CursorChannel. Is it worth adding the qxl field ? > +#ifdef DEBUG_CURSORS > +static int _cursor_count = 0; > +#endif > + This variable is unused, I'll remove it in the next patch. > static PipeItem *new_cursor_pipe_item(RedChannelClient *rcc, void *data, int > num) > { > CursorPipeItem *item = spice_malloc0(sizeof(CursorPipeItem)); > @@ -135,11 +142,15 @@ static void red_reset_cursor_cache(RedChannelClient > *rcc) > red_cursor_cache_reset(RCC_TO_CCC(rcc), CLIENT_CURSOR_CACHE_SIZE); > } > > -void cursor_channel_disconnect(RedChannel *channel) > +void cursor_channel_disconnect(CursorChannel *cursor) > { I agree for coherence with this type change. > + RedChannel *channel = (RedChannel *)cursor; > + > if (!channel || !red_channel_is_connected(channel)) { > return; > } > + > + /* TODO: use a class vdispose */ I don't understand this comment. > red_channel_apply_clients(channel, red_reset_cursor_cache); > red_channel_disconnect(channel); > } > @@ -147,7 +158,8 @@ void cursor_channel_disconnect(RedChannel *channel) > > static void put_cursor_pipe_item(CursorChannelClient *ccc, CursorPipeItem > *pipe_item) > { > - spice_assert(pipe_item); > + spice_return_if_fail(pipe_item); > + spice_return_if_fail(pipe_item->refs > 0); > Second check is checking a memory corruption, I suggest spice_error/spice_critical. > if (--pipe_item->refs) { > return; > @@ -155,7 +167,7 @@ static void put_cursor_pipe_item(CursorChannelClient > *ccc, CursorPipeItem *pipe_ > > spice_assert(!pipe_item_is_linked(&pipe_item->base)); > > - cursor_item_unref(red_worker_get_qxl(ccc->common.worker), > pipe_item->cursor_item); > + cursor_item_unref(pipe_item->cursor_item); > free(pipe_item); > } > > @@ -206,37 +218,37 @@ static void > cursor_channel_client_release_item_after_push(CursorChannelClient *c > static void red_marshall_cursor_init(RedChannelClient *rcc, SpiceMarshaller > *base_marshaller, > PipeItem *pipe_item) > { > - CursorChannel *cursor_channel; > + CursorChannel *cursor; > CursorChannelClient *ccc = RCC_TO_CCC(rcc); > SpiceMsgCursorInit msg; > AddBufInfo info; > > spice_assert(rcc); > - cursor_channel = SPICE_CONTAINEROF(rcc->channel, CursorChannel, > common.base); > + cursor = SPICE_CONTAINEROF(rcc->channel, CursorChannel, common.base); > > red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_INIT, NULL); > - msg.visible = cursor_channel->cursor_visible; > - msg.position = cursor_channel->cursor_position; > - msg.trail_length = cursor_channel->cursor_trail_length; > - msg.trail_frequency = cursor_channel->cursor_trail_frequency; > + msg.visible = cursor->cursor_visible; > + msg.position = cursor->cursor_position; > + msg.trail_length = cursor->cursor_trail_length; > + msg.trail_frequency = cursor->cursor_trail_frequency; > > - cursor_fill(ccc, &msg.cursor, cursor_channel->item, &info); > + cursor_fill(ccc, &msg.cursor, cursor->item, &info); > spice_marshall_msg_cursor_init(base_marshaller, &msg); > add_buf_from_info(base_marshaller, &info); > } > > static void cursor_marshall(RedChannelClient *rcc, > - SpiceMarshaller *m, CursorPipeItem *cursor_pipe_item) > + SpiceMarshaller *m, CursorPipeItem > *cursor_pipe_item) > { > - CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc->channel, > CursorChannel, common.base); > + CursorChannel *cursor = SPICE_CONTAINEROF(rcc->channel, CursorChannel, > common.base); > CursorChannelClient *ccc = RCC_TO_CCC(rcc); > - CursorItem *cursor = cursor_pipe_item->cursor_item; > + CursorItem *item = cursor_pipe_item->cursor_item; > PipeItem *pipe_item = &cursor_pipe_item->base; > RedCursorCmd *cmd; > > - spice_assert(cursor_channel); > + spice_return_if_fail(cursor); > Quite a big change in behavior. > - cmd = cursor->red_cursor; > + cmd = item->red_cursor; > switch (cmd->type) { > case QXL_CURSOR_MOVE: > { > @@ -253,9 +265,9 @@ static void cursor_marshall(RedChannelClient *rcc, > > red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_SET, > pipe_item); > cursor_set.position = cmd->u.set.position; > - cursor_set.visible = cursor_channel->cursor_visible; > + cursor_set.visible = cursor->cursor_visible; > > - cursor_fill(ccc, &cursor_set.cursor, cursor, &info); > + cursor_fill(ccc, &cursor_set.cursor, item, &info); > spice_marshall_msg_cursor_set(m, &cursor_set); > add_buf_from_info(m, &info); > break; > @@ -279,7 +291,7 @@ static void cursor_marshall(RedChannelClient *rcc, > } > > static inline void red_marshall_inval(RedChannelClient *rcc, > - SpiceMarshaller *base_marshaller, CacheItem *cach_item) > + SpiceMarshaller *base_marshaller, > CacheItem *cach_item) > { > SpiceMsgDisplayInvalOne inval_one; > > @@ -289,13 +301,6 @@ static inline void red_marshall_inval(RedChannelClient > *rcc, > spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one); > } > > -static void red_cursor_marshall_inval(RedChannelClient *rcc, > - SpiceMarshaller *m, CacheItem *cach_item) > -{ > - spice_assert(rcc); > - red_marshall_inval(rcc, m, cach_item); > -} > - Inlined, I agree with the change. > static void cursor_channel_send_item(RedChannelClient *rcc, PipeItem > *pipe_item) > { > SpiceMarshaller *m = red_channel_client_get_marshaller(rcc); > @@ -306,10 +311,10 @@ static void cursor_channel_send_item(RedChannelClient > *rcc, PipeItem *pipe_item) > cursor_marshall(rcc, m, SPICE_CONTAINEROF(pipe_item, CursorPipeItem, > base)); > break; > case PIPE_ITEM_TYPE_INVAL_ONE: > - red_cursor_marshall_inval(rcc, m, (CacheItem *)pipe_item); > + red_marshall_inval(rcc, m, (CacheItem *)pipe_item); > break; > case PIPE_ITEM_TYPE_VERB: > - red_marshall_verb(rcc, ((VerbItem*)pipe_item)->verb); > + red_marshall_verb(rcc, (VerbItem*)pipe_item); > break; > case PIPE_ITEM_TYPE_CURSOR_INIT: > red_reset_cursor_cache(rcc); > @@ -317,7 +322,7 @@ static void cursor_channel_send_item(RedChannelClient > *rcc, PipeItem *pipe_item) > break; > case PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE: > red_reset_cursor_cache(rcc); > - red_marshall_verb(rcc, SPICE_MSG_CURSOR_INVAL_ALL); > + red_channel_client_init_send_data(rcc, SPICE_MSG_CURSOR_INVAL_ALL, > NULL); > break; > default: > spice_error("invalid pipe item type"); > @@ -329,7 +334,9 @@ static void cursor_channel_send_item(RedChannelClient > *rcc, PipeItem *pipe_item) > > static CursorPipeItem *cursor_pipe_item_ref(CursorPipeItem *item) > { > - spice_assert(item); > + spice_return_val_if_fail(item, NULL); > + spice_return_val_if_fail(item->refs > 0, NULL); > + Same as above, more restrict check. > item->refs++; > return item; > } > @@ -338,8 +345,11 @@ static CursorPipeItem > *cursor_pipe_item_ref(CursorPipeItem *item) > static void cursor_channel_hold_pipe_item(RedChannelClient *rcc, PipeItem > *item) > { > CursorPipeItem *cursor_pipe_item; > - spice_assert(item); > + > + spice_return_if_fail(item); > + Why this change? > cursor_pipe_item = SPICE_CONTAINEROF(item, CursorPipeItem, base); > + /* TODO: refcnt at PipeItem instead ? */ I don't understand this comment. > cursor_pipe_item_ref(cursor_pipe_item); > } > > @@ -357,51 +367,56 @@ static void > cursor_channel_release_item(RedChannelClient *rcc, PipeItem *item, i > } > } > > -CursorChannel* cursor_channel_new(RedWorker *worker, int migrate) > +CursorChannel* cursor_channel_new(RedWorker *worker) > { The migrate field was not used, perhaps should be split and merged in another patch? I'm ok with it. > - CursorChannel* cursor; > + CursorChannel *cursor; > + RedChannel *channel = NULL; > + ChannelCbs cbs = { > + .on_disconnect = cursor_channel_client_on_disconnect, > + .send_item = cursor_channel_send_item, > + .hold_item = cursor_channel_hold_pipe_item, > + .release_item = cursor_channel_release_item > + }; > > spice_info("create cursor channel"); > - cursor = (CursorChannel *)__new_channel( > - worker, sizeof(CursorChannel), > - SPICE_CHANNEL_CURSOR, > - 0, > - cursor_channel_client_on_disconnect, > - cursor_channel_send_item, > - cursor_channel_hold_pipe_item, > - cursor_channel_release_item, > - red_channel_client_handle_message, > - NULL, > - NULL, > - NULL); > + channel = red_worker_new_channel(worker, sizeof(CursorChannel), > + SPICE_CHANNEL_CURSOR, 0, > + &cbs, > red_channel_client_handle_message); > > + cursor = (CursorChannel *)channel; > cursor->cursor_visible = TRUE; > cursor->mouse_mode = SPICE_MOUSE_MODE_SERVER; > > return cursor; > } > > -CursorChannelClient *cursor_channel_client_new(CommonChannel *common, > - RedClient *client, RedsStream > *stream, > +CursorChannelClient* cursor_channel_client_new(CursorChannel *cursor, > RedClient *client, RedsStream *stream, > int mig_target, > uint32_t *common_caps, int > num_common_caps, > uint32_t *caps, int num_caps) > { > + spice_return_val_if_fail(cursor, NULL); > + spice_return_val_if_fail(client, NULL); > + spice_return_val_if_fail(stream, NULL); > + spice_return_val_if_fail(!num_common_caps || common_caps, NULL); > + spice_return_val_if_fail(!num_caps || caps, NULL); > + > CursorChannelClient *ccc = > - (CursorChannelClient*)common_channel_client_create( > - sizeof(CursorChannelClient), common, client, stream, > - mig_target, > - FALSE, > - common_caps, > - num_common_caps, > - caps, > - num_caps); > - > - if (!ccc) { > - return NULL; > - } > + (CursorChannelClient*)common_channel_new_client(&cursor->common, > + > sizeof(CursorChannelClient), > + client, stream, > + mig_target, > + FALSE, > + common_caps, > + num_common_caps, > + caps, > + num_caps); > + spice_return_val_if_fail(ccc != NULL, NULL); > + > ring_init(&ccc->cursor_cache_lru); > ccc->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE; > + > + > return ccc; > } > > @@ -411,7 +426,11 @@ void cursor_channel_process_cmd(CursorChannel *cursor, > RedCursorCmd *cursor_cmd, > CursorItem *cursor_item; > int cursor_show = FALSE; > > - cursor_item = cursor_item_new(cursor_cmd, group_id); > + spice_return_if_fail(cursor); > + spice_return_if_fail(cursor_cmd); > + > + cursor_item = cursor_item_new(red_worker_get_qxl(cursor->common.worker), > + cursor_cmd, group_id); > > switch (cursor_cmd->type) { > case QXL_CURSOR_SET: > @@ -431,34 +450,40 @@ void cursor_channel_process_cmd(CursorChannel *cursor, > RedCursorCmd *cursor_cmd, > cursor->cursor_trail_frequency = cursor_cmd->u.trail.frequency; > break; > default: > - spice_error("invalid cursor command %u", cursor_cmd->type); > + spice_warning("invalid cursor command %u", cursor_cmd->type); > + return; If command came from VM I agree to ignore the message. If not I would prefer the error. > } > > - if (red_channel_is_connected(&cursor->common.base) && > (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER || > - cursor_cmd->type != QXL_CURSOR_MOVE || > cursor_show)) { > - red_channel_pipes_new_add(&cursor->common.base, > new_cursor_pipe_item, > - (void*)cursor_item); > + if (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER > + || cursor_cmd->type != QXL_CURSOR_MOVE > + || cursor_show) { Why the connection check was removed? > + red_channel_pipes_new_add(&cursor->common.base, > + new_cursor_pipe_item, cursor_item); > } > - cursor_item_unref(red_worker_get_qxl(cursor->common.worker), > cursor_item); > + > + cursor_item_unref(cursor_item); > } > > void cursor_channel_reset(CursorChannel *cursor) > { > + RedChannel *channel = (RedChannel *)cursor; > + RedChannel *channel = &cursor->common.base; is more type safe. Actually just style change. > + spice_return_if_fail(cursor); > + > cursor_set_item(cursor, NULL); > cursor->cursor_visible = TRUE; > cursor->cursor_position.x = cursor->cursor_position.y = 0; > cursor->cursor_trail_length = cursor->cursor_trail_frequency = 0; > > - if (red_channel_is_connected(&cursor->common.base)) { > - red_channel_pipes_add_type(&cursor->common.base, > - PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE); > + if (red_channel_is_connected(channel)) { > + red_channel_pipes_add_type(&cursor->common.base, > PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE); > if (!cursor->common.during_target_migrate) { > red_pipes_add_verb(&cursor->common.base, > SPICE_MSG_CURSOR_RESET); > } > if (!red_channel_wait_all_sent(&cursor->common.base, > DISPLAY_CLIENT_TIMEOUT)) { > red_channel_apply_clients(&cursor->common.base, > - > red_channel_client_disconnect_if_pending_send); > + > red_channel_client_disconnect_if_pending_send); > } > } > } > diff --git a/server/cursor-channel.h b/server/cursor-channel.h > index 2c19859..e1ef4b6 100644 > --- a/server/cursor-channel.h > +++ b/server/cursor-channel.h > @@ -32,7 +32,14 @@ > #define CURSOR_CACHE_HASH_MASK (CURSOR_CACHE_HASH_SIZE - 1) > #define CURSOR_CACHE_HASH_KEY(id) ((id) & CURSOR_CACHE_HASH_MASK) > > +enum { > + PIPE_ITEM_TYPE_CURSOR = PIPE_ITEM_TYPE_COMMON_LAST, > + PIPE_ITEM_TYPE_CURSOR_INIT, > + PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE, > +}; > + > typedef struct CursorItem { > + QXLInstance *qxl; > uint32_t group_id; > int refs; > RedCursorCmd *red_cursor; > @@ -77,20 +84,20 @@ typedef struct CursorChannel { > > G_STATIC_ASSERT(sizeof(CursorItem) <= QXL_CURSUR_DEVICE_DATA_SIZE); > > -CursorChannel* cursor_channel_new (RedWorker *worker, int > migrate); > -void cursor_channel_disconnect (RedChannel *channel); > +CursorChannel* cursor_channel_new (RedWorker *worker); > +void cursor_channel_disconnect (CursorChannel *cursor); > void cursor_channel_reset (CursorChannel *cursor); > void cursor_channel_process_cmd (CursorChannel *cursor, > RedCursorCmd *cursor_cmd, > uint32_t group_id); > > -CursorItem* cursor_item_new (RedCursorCmd *cmd, uint32_t > group_id); > -void cursor_item_unref (QXLInstance *qxl, > CursorItem *cursor); > - > +CursorChannelClient* cursor_channel_client_new (CursorChannel *cursor, > + RedClient *client, > RedsStream *stream, > + int mig_target, > + uint32_t *common_caps, int > num_common_caps, > + uint32_t *caps, int > num_caps); > > -CursorChannelClient *cursor_channel_client_new(CommonChannel *common, > - RedClient *client, RedsStream > *stream, > - int mig_target, > - uint32_t *common_caps, int > num_common_caps, > - uint32_t *caps, int > num_caps); > +CursorItem* cursor_item_new (QXLInstance *qxl, > RedCursorCmd *cmd, uint32_t group_id); > +CursorItem* cursor_item_ref (CursorItem *cursor); > +void cursor_item_unref (CursorItem *cursor); > > #endif /* CURSOR_CHANNEL_H_ */ > diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c > index 43f061d..3fc2c1c 100644 > --- a/server/red_dispatcher.c > +++ b/server/red_dispatcher.c > @@ -160,6 +160,7 @@ static void red_dispatcher_set_cursor_peer(RedChannel > *channel, RedClient *clien > memcpy(payload.common_caps, common_caps, > sizeof(uint32_t)*num_common_caps); > memcpy(payload.caps, caps, sizeof(uint32_t)*num_caps); > > + /* TODO serialize it all, no dangling pointers */ > dispatcher_send_message(&dispatcher->dispatcher, > RED_WORKER_MESSAGE_CURSOR_CONNECT, > &payload); This comment is not related to the commit message, I would remove it. > diff --git a/server/red_worker.c b/server/red_worker.c > index 8ae0878..817ff34 100644 > --- a/server/red_worker.c > +++ b/server/red_worker.c > @@ -265,6 +265,23 @@ struct SpiceWatch { > void *watch_func_opaque; > }; > > +enum { > + PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_COMMON_LAST, > + PIPE_ITEM_TYPE_IMAGE, > + PIPE_ITEM_TYPE_STREAM_CREATE, > + PIPE_ITEM_TYPE_STREAM_CLIP, > + PIPE_ITEM_TYPE_STREAM_DESTROY, > + PIPE_ITEM_TYPE_UPGRADE, > + PIPE_ITEM_TYPE_MIGRATE_DATA, > + PIPE_ITEM_TYPE_PIXMAP_SYNC, > + PIPE_ITEM_TYPE_PIXMAP_RESET, > + PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE, > + PIPE_ITEM_TYPE_CREATE_SURFACE, > + PIPE_ITEM_TYPE_DESTROY_SURFACE, > + PIPE_ITEM_TYPE_MONITORS_CONFIG, > + PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT, > +}; > + > #define MAX_LZ_ENCODERS MAX_CACHE_CLIENTS > > typedef struct SurfaceCreateItem { > @@ -720,7 +737,6 @@ static void > display_channel_client_release_item_before_push(DisplayChannelClient > PipeItem *item); > static void > display_channel_client_release_item_after_push(DisplayChannelClient *dcc, > PipeItem *item); > - > static void red_push_monitors_config(DisplayChannelClient *dcc); > > /* > @@ -764,7 +780,6 @@ static void red_push_monitors_config(DisplayChannelClient > *dcc); > DisplayChannel, common.base) > > #define RCC_TO_DCC(rcc) SPICE_CONTAINEROF((rcc), DisplayChannelClient, > common.base) > -#define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), CursorChannelClient, > common.base) > > > > @@ -4192,12 +4207,11 @@ static int red_process_cursor(RedWorker *worker, > uint32_t max_pipe_size, int *ri > free(cursor); > break; > } > - > cursor_channel_process_cmd(worker->cursor_channel, cursor, > ext_cmd.group_id); > break; > } > default: > - spice_error("bad command type"); > + spice_warning("bad command type"); > } Where does the command came? If from VM I'm ok, if from internal I would prefer an error. > n++; > } > @@ -4471,7 +4485,6 @@ static void marshaller_add_compressed(SpiceMarshaller > *m, > } while (max); > } > > - I would remove these spurious space changes. > static void fill_base(SpiceMarshaller *base_marshaller, Drawable *drawable) > { > SpiceMsgDisplayBase base; > @@ -8293,6 +8306,8 @@ static void > red_display_marshall_stream_end(RedChannelClient *rcc, > spice_marshall_msg_display_stream_destroy(base_marshaller, &destroy); > } > > +#define RCC_TO_CCC(rcc) SPICE_CONTAINEROF((rcc), CursorChannelClient, > common.base) > + This macro is just moved, I'll remove this change. > static void red_marshall_surface_create(RedChannelClient *rcc, > SpiceMarshaller *base_marshaller, SpiceMsgSurfaceCreate *surface_create) > { > @@ -8361,17 +8376,6 @@ static void > red_marshall_stream_activate_report(RedChannelClient *rcc, > spice_marshall_msg_display_stream_activate_report(base_marshaller, > &msg); > } > > -static inline void red_marshall_inval(RedChannelClient *rcc, > - SpiceMarshaller *base_marshaller, > CacheItem *cach_item) > -{ > - SpiceMsgDisplayInvalOne inval_one; > - > - red_channel_client_init_send_data(rcc, cach_item->inval_type, NULL); > - inval_one.id = *(uint64_t *)&cach_item->id; > - > - spice_marshall_msg_cursor_inval_one(base_marshaller, &inval_one); > -} > - > static void display_channel_send_item(RedChannelClient *rcc, PipeItem > *pipe_item) > { > SpiceMarshaller *m = red_channel_client_get_marshaller(rcc); > @@ -8384,9 +8388,6 @@ static void display_channel_send_item(RedChannelClient > *rcc, PipeItem *pipe_item > marshall_qxl_drawable(rcc, m, dpi); > break; > } > - case PIPE_ITEM_TYPE_INVAL_ONE: > - red_marshall_inval(rcc, m, (CacheItem *)pipe_item); > - break; > case PIPE_ITEM_TYPE_STREAM_CREATE: { > StreamAgent *agent = SPICE_CONTAINEROF(pipe_item, StreamAgent, > create_item); > red_display_marshall_stream_start(rcc, m, agent); > @@ -8406,7 +8407,7 @@ static void display_channel_send_item(RedChannelClient > *rcc, PipeItem *pipe_item > red_display_marshall_upgrade(rcc, m, (UpgradeItem *)pipe_item); > break; > case PIPE_ITEM_TYPE_VERB: > - red_marshall_verb(rcc, ((VerbItem*)pipe_item)->verb); > + red_marshall_verb(rcc, (VerbItem*)pipe_item); > break; > case PIPE_ITEM_TYPE_MIGRATE_DATA: > display_channel_marshall_migrate_data(rcc, m); > @@ -8422,7 +8423,7 @@ static void display_channel_send_item(RedChannelClient > *rcc, PipeItem *pipe_item > break; > case PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE: > red_reset_palette_cache(dcc); > - red_marshall_verb(rcc, SPICE_MSG_DISPLAY_INVAL_ALL_PALETTES); > + red_channel_client_init_send_data(rcc, > SPICE_MSG_DISPLAY_INVAL_ALL_PALETTES, NULL); > break; > case PIPE_ITEM_TYPE_CREATE_SURFACE: { > SurfaceCreateItem *surface_create = SPICE_CONTAINEROF(pipe_item, > SurfaceCreateItem, > @@ -8908,7 +8909,7 @@ static inline void flush_cursor_commands(RedWorker > *worker) > red_channel_send(channel); > if (red_now() >= end_time) { > spice_warning("flush cursor timeout"); > - cursor_channel_disconnect(channel); > + cursor_channel_disconnect(worker->cursor_channel); > worker->cursor_channel = NULL; > } else { > sleep_count++; > @@ -9520,16 +9521,16 @@ SpiceCoreInterface worker_core = { > .watch_remove = worker_watch_remove, > }; > > -CommonChannelClient *common_channel_client_create(int size, > - CommonChannel *common, > - RedClient *client, > - RedsStream *stream, > - int mig_target, > - int monitor_latency, > - uint32_t *common_caps, > - int num_common_caps, > - uint32_t *caps, > - int num_caps) > +CommonChannelClient *common_channel_new_client(CommonChannel *common, > + int size, Why changing argument order? > + RedClient *client, > + RedsStream *stream, > + int mig_target, > + int monitor_latency, > + uint32_t *common_caps, > + int num_common_caps, > + uint32_t *caps, > + int num_caps) > { > RedChannelClient *rcc = > red_channel_client_create(size, &common->base, client, stream, > monitor_latency, > @@ -9557,8 +9558,8 @@ DisplayChannelClient > *display_channel_client_create(CommonChannel *common, > uint32_t *caps, int > num_caps) > { > DisplayChannelClient *dcc = > - (DisplayChannelClient*)common_channel_client_create( > - sizeof(DisplayChannelClient), common, client, stream, > + (DisplayChannelClient*)common_channel_new_client( > + common, sizeof(DisplayChannelClient), client, stream, > mig_target, > TRUE, > common_caps, num_common_caps, > @@ -9572,38 +9573,30 @@ DisplayChannelClient > *display_channel_client_create(CommonChannel *common, > return dcc; > } > > -RedChannel *__new_channel(RedWorker *worker, int size, uint32_t > channel_type, > - int migration_flags, > - channel_disconnect_proc on_disconnect, > - channel_send_pipe_item_proc send_item, > - channel_hold_pipe_item_proc hold_item, > - channel_release_pipe_item_proc release_item, > - channel_handle_parsed_proc handle_parsed, > - channel_handle_migrate_flush_mark_proc > handle_migrate_flush_mark, > - channel_handle_migrate_data_proc > handle_migrate_data, > - channel_handle_migrate_data_get_serial_proc > migrate_get_serial) > +RedChannel *red_worker_new_channel(RedWorker *worker, int size, > + uint32_t channel_type, int > migration_flags, > + ChannelCbs *channel_cbs, > + channel_handle_parsed_proc handle_parsed) > { > RedChannel *channel = NULL; > CommonChannel *common; > - ChannelCbs channel_cbs = { NULL, }; > - > - channel_cbs.config_socket = common_channel_config_socket; > - channel_cbs.on_disconnect = on_disconnect; > - channel_cbs.send_item = send_item; > - channel_cbs.hold_item = hold_item; > - channel_cbs.release_item = release_item; > - channel_cbs.alloc_recv_buf = common_alloc_recv_buf; > - channel_cbs.release_recv_buf = common_release_recv_buf; > - channel_cbs.handle_migrate_flush_mark = handle_migrate_flush_mark; > - channel_cbs.handle_migrate_data = handle_migrate_data; > - channel_cbs.handle_migrate_data_get_serial = migrate_get_serial; > + > + spice_return_val_if_fail(worker, NULL); > + spice_return_val_if_fail(channel_cbs, NULL); > + spice_return_val_if_fail(!channel_cbs->config_socket, NULL); > + spice_return_val_if_fail(!channel_cbs->alloc_recv_buf, NULL); > + spice_return_val_if_fail(!channel_cbs->release_recv_buf, NULL); > + > + channel_cbs->config_socket = common_channel_config_socket; > + channel_cbs->alloc_recv_buf = common_alloc_recv_buf; > + channel_cbs->release_recv_buf = common_release_recv_buf; > > channel = red_channel_create_parser(size, &worker_core, > channel_type, worker->qxl->id, > TRUE /* handle_acks */, > spice_get_client_channel_parser(channel_type, > NULL), > handle_parsed, > - &channel_cbs, > + channel_cbs, > migration_flags); > common = (CommonChannel *)channel; > if (!channel) { > @@ -9723,7 +9716,6 @@ static void > display_channel_client_release_item_before_push(DisplayChannelClient > free(item); > break; > } > - case PIPE_ITEM_TYPE_INVAL_ONE: > case PIPE_ITEM_TYPE_VERB: > case PIPE_ITEM_TYPE_MIGRATE_DATA: > case PIPE_ITEM_TYPE_PIXMAP_SYNC: > @@ -9753,25 +9745,26 @@ static void > display_channel_release_item(RedChannelClient *rcc, PipeItem *item, > static void display_channel_create(RedWorker *worker, int migrate) > { > DisplayChannel *display_channel; > + ChannelCbs cbs = { > + .on_disconnect = display_channel_client_on_disconnect, > + .send_item = display_channel_send_item, > + .hold_item = display_channel_hold_pipe_item, > + .release_item = display_channel_release_item, > + .handle_migrate_flush_mark = display_channel_handle_migrate_mark, > + .handle_migrate_data = display_channel_handle_migrate_data, > + .handle_migrate_data_get_serial = > display_channel_handle_migrate_data_get_serial > + }; > > if (worker->display_channel) { > return; > } > > spice_info("create display channel"); > - if (!(worker->display_channel = (DisplayChannel *)__new_channel( > + if (!(worker->display_channel = (DisplayChannel > *)red_worker_new_channel( > worker, sizeof(*display_channel), > SPICE_CHANNEL_DISPLAY, > SPICE_MIGRATE_NEED_FLUSH | SPICE_MIGRATE_NEED_DATA_TRANSFER, > - display_channel_client_on_disconnect, > - display_channel_send_item, > - display_channel_hold_pipe_item, > - display_channel_release_item, > - display_channel_handle_message, > - display_channel_handle_migrate_mark, > - display_channel_handle_migrate_data, > - display_channel_handle_migrate_data_get_serial > - ))) { > + &cbs, display_channel_handle_message))) { > spice_warning("failed to create display channel"); > return; > } > @@ -9906,29 +9899,6 @@ static void handle_new_display_channel(RedWorker > *worker, RedClient *client, Red > on_new_display_channel_client(dcc); > } > > -static void red_migrate_cursor(RedWorker *worker, RedChannelClient *rcc) > -{ > - if (red_channel_client_is_connected(rcc)) { > - red_channel_client_pipe_add_type(rcc, > - PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE); > - red_channel_client_default_migrate(rcc); > - } > -} > - > -static void on_new_cursor_channel(RedWorker *worker, RedChannelClient *rcc) > -{ > - CursorChannel *channel = worker->cursor_channel; > - > - spice_assert(channel); > - red_channel_client_ack_zero_messages_window(rcc); > - red_channel_client_push_set_ack(rcc); > - // TODO: why do we check for context.canvas? defer this to after display > cc is connected > - // and test it's canvas? this is just a test to see if there is an > active renderer? > - if (worker->surfaces[0].context.canvas && > !channel->common.during_target_migrate) { > - red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_CURSOR_INIT); > - } > -} > - These function was inlined directly. Just style, I'm fine with this change. Perhaps not really related. > static void red_connect_cursor(RedWorker *worker, RedClient *client, > RedsStream *stream, > int migrate, > uint32_t *common_caps, int num_common_caps, > @@ -9937,24 +9907,28 @@ static void red_connect_cursor(RedWorker *worker, > RedClient *client, RedsStream > CursorChannel *channel; > CursorChannelClient *ccc; > > - if (worker->cursor_channel == NULL) { > - spice_warning("cursor channel was not created"); > - return; > - } > + spice_return_if_fail(worker->cursor_channel != NULL); > + > channel = worker->cursor_channel; > spice_info("add cursor channel client"); > - ccc = cursor_channel_client_new(&channel->common, client, stream, > + ccc = cursor_channel_client_new(channel, client, stream, > migrate, > common_caps, num_common_caps, > caps, num_caps); > - if (!ccc) { > - return; > - } > + spice_return_if_fail(ccc != NULL); > #ifdef RED_STATISTICS > channel->stat = stat_add_node(worker->stat, "cursor_channel", TRUE); > channel->common.base.out_bytes_counter = stat_add_counter(channel->stat, > "out_bytes", TRUE); > #endif > - on_new_cursor_channel(worker, &ccc->common.base); > + > + RedChannelClient *rcc = &ccc->common.base; > + red_channel_client_ack_zero_messages_window(rcc); > + red_channel_client_push_set_ack(rcc); > + // TODO: why do we check for context.canvas? defer this to after display > cc is connected > + // and test it's canvas? this is just a test to see if there is an > active renderer? > + if (worker->surfaces[0].context.canvas && > !channel->common.during_target_migrate) { > + red_channel_client_pipe_add_type(rcc, PIPE_ITEM_TYPE_CURSOR_INIT); > + } > } > > static void surface_dirty_region_to_rects(RedSurface *surface, > @@ -10296,10 +10270,9 @@ static void dev_create_primary_surface(RedWorker > *worker, uint32_t surface_id, > red_channel_push(&worker->display_channel->common.base); > } > > - if (cursor_is_connected(worker) && > !worker->cursor_channel->common.during_target_migrate) { > + if (!worker->cursor_channel->common.during_target_migrate) The connection check was removed without explanation > red_channel_pipes_add_type(&worker->cursor_channel->common.base, > PIPE_ITEM_TYPE_CURSOR_INIT); > - } > } > > void handle_dev_create_primary_surface(void *opaque, void *payload) > @@ -10498,7 +10471,9 @@ void handle_dev_oom(void *opaque, void *payload) > > void handle_dev_reset_cursor(void *opaque, void *payload) > { > - cursor_channel_reset(((RedWorker*)opaque)->cursor_channel); > + RedWorker *worker = opaque; > + > + cursor_channel_reset(worker->cursor_channel); > } > > void handle_dev_reset_image_cache(void *opaque, void *payload) > @@ -10642,10 +10617,12 @@ void handle_dev_cursor_channel_create(void *opaque, > void *payload) > RedWorker *worker = opaque; > RedChannel *red_channel; > > - // TODO: handle seemless migration. Temp, setting migrate to FALSE > if (!worker->cursor_channel) { > - worker->cursor_channel = cursor_channel_new(worker, FALSE); > + worker->cursor_channel = cursor_channel_new(worker); > + } else { > + spice_warning("cursor channel already created"); > } > + > red_channel = &worker->cursor_channel->common.base; > send_data(worker->channel, &red_channel, sizeof(RedChannel *)); > } > @@ -10672,19 +10649,20 @@ void handle_dev_cursor_disconnect(void *opaque, > void *payload) > RedChannelClient *rcc = msg->rcc; > > spice_info("disconnect cursor client"); > - spice_assert(rcc); > red_channel_client_disconnect(rcc); > } > > void handle_dev_cursor_migrate(void *opaque, void *payload) > { > RedWorkerMessageCursorMigrate *msg = payload; > - RedWorker *worker = opaque; > RedChannelClient *rcc = msg->rcc; > > spice_info("migrate cursor client"); > - spice_assert(rcc); > - red_migrate_cursor(worker, rcc); > + if (!red_channel_client_is_connected(rcc)) > + return; > + > + red_channel_client_pipe_add_type(rcc, > PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE); > + red_channel_client_default_migrate(rcc); > } > > void handle_dev_set_compression(void *opaque, void *payload) > @@ -10761,8 +10739,10 @@ void handle_dev_set_mouse_mode(void *opaque, void > *payload) > RedWorkerMessageSetMouseMode *msg = payload; > RedWorker *worker = opaque; > > + spice_info("mouse mode %u", msg->mode); > + spice_return_if_fail(worker->cursor_channel); > + > worker->cursor_channel->mouse_mode = msg->mode; > - spice_info("mouse mode %u", worker->cursor_channel->mouse_mode); > } > > void handle_dev_add_memslot_async(void *opaque, void *payload) > diff --git a/server/red_worker.h b/server/red_worker.h > index 502283e..60c326a 100644 > --- a/server/red_worker.h > +++ b/server/red_worker.h > @@ -32,6 +32,7 @@ typedef struct CommonChannelClient { > int is_low_bandwidth; > } CommonChannelClient; > > + > #define DISPLAY_CLIENT_TIMEOUT 30000000000ULL //nano > > #define CHANNEL_RECEIVE_BUF_SIZE 1024 > @@ -48,25 +49,10 @@ typedef struct CommonChannel { > } CommonChannel; > > enum { > - PIPE_ITEM_TYPE_DRAW = PIPE_ITEM_TYPE_CHANNEL_BASE, > + PIPE_ITEM_TYPE_VERB = PIPE_ITEM_TYPE_CHANNEL_BASE, > PIPE_ITEM_TYPE_INVAL_ONE, > - PIPE_ITEM_TYPE_CURSOR, > - PIPE_ITEM_TYPE_CURSOR_INIT, > - PIPE_ITEM_TYPE_IMAGE, > - PIPE_ITEM_TYPE_STREAM_CREATE, > - PIPE_ITEM_TYPE_STREAM_CLIP, > - PIPE_ITEM_TYPE_STREAM_DESTROY, > - PIPE_ITEM_TYPE_UPGRADE, > - PIPE_ITEM_TYPE_VERB, > - PIPE_ITEM_TYPE_MIGRATE_DATA, > - PIPE_ITEM_TYPE_PIXMAP_SYNC, > - PIPE_ITEM_TYPE_PIXMAP_RESET, > - PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE, > - PIPE_ITEM_TYPE_INVAL_PALETTE_CACHE, > - PIPE_ITEM_TYPE_CREATE_SURFACE, > - PIPE_ITEM_TYPE_DESTROY_SURFACE, > - PIPE_ITEM_TYPE_MONITORS_CONFIG, > - PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT, > + > + PIPE_ITEM_TYPE_COMMON_LAST > }; > > typedef struct VerbItem { > @@ -74,10 +60,9 @@ typedef struct VerbItem { > uint16_t verb; > } VerbItem; > > -static inline void red_marshall_verb(RedChannelClient *rcc, uint16_t verb) > +static inline void red_marshall_verb(RedChannelClient *rcc, VerbItem *item) > { > - spice_assert(rcc); > - red_channel_client_init_send_data(rcc, verb, NULL); > + red_channel_client_init_send_data(rcc, item->verb, NULL); > } > > static inline void red_pipe_add_verb(RedChannelClient* rcc, uint16_t verb) > @@ -103,7 +88,6 @@ static inline void red_pipe_add_verb(RedChannelClient* > rcc, uint16_t verb) > #define RCC_FOREACH_SAFE(link, next, rcc, channel) \ > SAFE_FOREACH(link, next, channel, &(channel)->clients, rcc, > LINK_TO_RCC(link)) > > - > static inline void red_pipes_add_verb(RedChannel *channel, uint16_t verb) > { > RedChannelClient *rcc; > @@ -114,30 +98,25 @@ static inline void red_pipes_add_verb(RedChannel > *channel, uint16_t verb) > } > } > > + > RedWorker* red_worker_new(QXLInstance *qxl, RedDispatcher *red_dispatcher); > bool red_worker_run(RedWorker *worker); > QXLInstance* red_worker_get_qxl(RedWorker *worker); > > -CommonChannelClient *common_channel_client_create(int size, > - CommonChannel *common, > - RedClient *client, > - RedsStream *stream, > - int mig_target, > - int monitor_latency, > - uint32_t *common_caps, > - int num_common_caps, > - uint32_t *caps, > - int num_caps); > - > -RedChannel *__new_channel(RedWorker *worker, int size, uint32_t > channel_type, > - int migration_flags, > - channel_disconnect_proc on_disconnect, > - channel_send_pipe_item_proc send_item, > - channel_hold_pipe_item_proc hold_item, > - channel_release_pipe_item_proc release_item, > - channel_handle_parsed_proc handle_parsed, > - channel_handle_migrate_flush_mark_proc > handle_migrate_flush_mark, > - channel_handle_migrate_data_proc > handle_migrate_data, > - channel_handle_migrate_data_get_serial_proc > migrate_get_serial); > +RedChannel *red_worker_new_channel(RedWorker *worker, int size, > + uint32_t channel_type, int > migration_flags, > + ChannelCbs *channel_cbs, > + channel_handle_parsed_proc > handle_parsed); > + > +CommonChannelClient *common_channel_new_client(CommonChannel *common, > + int size, > + RedClient *client, > + RedsStream *stream, > + int mig_target, > + int monitor_latency, > + uint32_t *common_caps, > + int num_common_caps, > + uint32_t *caps, > + int num_caps); > > #endif This patch require more attention and more review. Frediano _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel