Re: [PATCH 05/10] Change some asserts to warnings

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Nov 3, 2015 at 11:05 AM, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>>
>> 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?

I am going to do it.
And I changed my mind about the "spice_warn_if_reached()" changes that
I suggested because then you would lose the info provided by the
spice_warning().

Best Regards,
--
Fabiano Fidêncio
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]