>
> On Mon, Nov 2, 2015 at 10:56 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxx>
> >
> > Various changes in RedWorker and CursorChannel related to error and
> > warning messages.
> >
> > Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > ---
> > server/cursor-channel.c | 28 ++++++++++++++++++++--------
> > server/red_worker.c | 22 ++++++++++------------
> > server/red_worker.h | 1 -
> > 3 files changed, 30 insertions(+), 21 deletions(-)
> >
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index d145f86..6a1fcea 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -152,7 +152,8 @@ void cursor_channel_disconnect(CursorChannel
> > *cursor_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);
> >
> > if (--pipe_item->refs) {
> > return;
> > @@ -239,7 +240,7 @@ static void cursor_marshall(RedChannelClient *rcc,
> > PipeItem *pipe_item = &cursor_pipe_item->base;
> > RedCursorCmd *cmd;
> >
> > - spice_assert(cursor_channel);
> > + spice_return_if_fail(cursor_channel);
> >
> > cmd = item->red_cursor;
> > switch (cmd->type) {
> > @@ -327,7 +328,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);
> > +
> > item->refs++;
> > return item;
> > }
> > @@ -336,7 +339,9 @@ 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);
> > +
> > cursor_pipe_item = SPICE_CONTAINEROF(item, CursorPipeItem, base);
> > cursor_pipe_item_ref(cursor_pipe_item);
> > }
> > @@ -383,6 +388,12 @@ CursorChannelClient*
> > cursor_channel_client_new(CursorChannel *cursor, RedClient
> > 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_new_client(&cursor->common,
> > sizeof(CursorChannelClient),
> > @@ -393,11 +404,11 @@ CursorChannelClient*
> > cursor_channel_client_new(CursorChannel *cursor, RedClient
> > num_common_caps,
> > caps,
> > num_caps);
> > - if (!ccc) {
> > - return NULL;
> > - }
> > + spice_return_val_if_fail(ccc != NULL, NULL);
> > +
> > ring_init(&ccc->cursor_cache_lru);
> > ccc->cursor_cache_available = CLIENT_CURSOR_CACHE_SIZE;
> > +
> > return ccc;
> > }
> >
> > @@ -431,7 +442,8 @@ 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;
> > }
>
> I would go for spice_warn_if_reached() here.
>
> >
> > if (red_channel_is_connected(&cursor->common.base) &&
> > diff --git a/server/red_worker.c b/server/red_worker.c
> > index c3b5c36..339b353 100644
> > --- a/server/red_worker.c
> > +++ b/server/red_worker.c
> > @@ -4169,7 +4169,7 @@ static int red_process_cursor(RedWorker *worker,
> > uint32_t max_pipe_size, int *ri
> > break;
> > }
> > default:
> > - spice_error("bad command type");
> > + spice_warning("bad command type");
> > }
>
> Same here.
>
> > n++;
> > }
> > @@ -9769,19 +9769,15 @@ 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, 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);
> > @@ -10485,10 +10481,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);
> > + } else {
> > + spice_warning("cursor channel already created");
> > }
> > +
> > red_channel = &worker->cursor_channel->common.base;
> > send_data(worker->channel, &red_channel, sizeof(RedChannel *));
> > }
> > @@ -10515,7 +10513,6 @@ void handle_dev_cursor_disconnect(void *opaque,
> > void *payload)
> > RedChannelClient *rcc = msg->rcc;
> >
> > spice_info("disconnect cursor client");
> > - spice_assert(rcc);
>
> I would add a spice_return_if_fail() here or add a check for NULL
> inside the red_channel_client_disconnect()
>
> > red_channel_client_disconnect(rcc);
> > }
> >
> > @@ -10525,7 +10522,6 @@ void handle_dev_cursor_migrate(void *opaque, void
> > *payload)
> > RedChannelClient *rcc = msg->rcc;
> >
> > spice_info("migrate cursor client");
> > - spice_assert(rcc);
>
> I would add a spice_return_if_fail() here or add a check for NULL
> inside the red_channel_client_is_connected()
>
>
> > if (!red_channel_client_is_connected(rcc))
> > return;
> >
> > @@ -10607,8 +10603,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 aa97707..df52abd 100644
> > --- a/server/red_worker.h
> > +++ b/server/red_worker.h
> > @@ -61,7 +61,6 @@ typedef struct VerbItem {
> >
> > static inline void red_marshall_verb(RedChannelClient *rcc, VerbItem
> > *item)
> > {
> > - spice_assert(rcc);
>
> Just a note, removing this assert here is okay, as rcc is verified in
> red_channel_client_no_item_being_sent(), inside
> red_channel_client_init_send_data().
>
> > red_channel_client_init_send_data(rcc, item->verb, NULL);
> > }
> >
> > --
> > 2.4.3
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> ACK with these changes.
>
Can you post an updated patch?
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel