[Spice-devel] [PATCH 3.11/12] Change some asserts to warnings
Frediano Ziglio
fziglio at redhat.com
Fri Oct 30 00:55:17 PDT 2015
>
> From: Marc-André Lureau <marcandre.lureau at gmail.com>
>
> Various changes in RedWorker and CursorChannel related to error and
> warning messages.
>
> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
> ---
> 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 5222bd8..ecb49b2 100644
> --- a/server/cursor-channel.c
> +++ b/server/cursor-channel.c
> @@ -152,7 +152,8 @@ void cursor_channel_disconnect(CursorChannel *cursor)
>
> 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 test should be more strict, we are detecting memory corruption!
>
> 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);
Test for NULL should be done on rcc, there is no reason as rcc == NULL or rcc->channel == NULL
imply cursor_channel NULL (assuming just second condition is not good either).
Would even better to set ccc and cursor_channel after the test.
>
> cmd = cursor->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);
> +
See above.
> 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;
> }
>
Not fully agree with this change. Is cursor command internally generated?
> if (red_channel_is_connected(&cursor->common.base) &&
> (cursor->mouse_mode == SPICE_MOUSE_MODE_SERVER ||
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 2967c91..b93676e 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -4214,7 +4214,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");
> }
> n++;
> }
Same as previous comment.
> @@ -9909,19 +9909,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);
> @@ -10622,10 +10618,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
Could comment could be removed in the "Remove unused parameter from cursor_channel_new()"
patch.
> 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 *));
> }
> @@ -10652,7 +10650,6 @@ 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);
> }
>
> @@ -10662,7 +10659,6 @@ void handle_dev_cursor_migrate(void *opaque, void
> *payload)
> RedChannelClient *rcc = msg->rcc;
>
> spice_info("migrate cursor client");
> - spice_assert(rcc);
> if (!red_channel_client_is_connected(rcc))
> return;
>
> @@ -10744,8 +10740,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);
> +
Is it common/expected to not have a cursor_channel? If is normal perhaps
logging this is useless (spice_return_if_fail does some logging).
Also the behavior changed (in a previous patch) slightly as before we were
able to set mouse_mode even not having a 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);
> red_channel_client_init_send_data(rcc, item->verb, NULL);
> }
>
> --
> 2.4.3
>
Frediano
More information about the Spice-devel
mailing list