[Spice-devel] [PATCH 09/15] worker: remove cursor channel asserts

[Victor Toso]

Hi,

On Mon, Nov 09, 2015 at 03:25:16PM -0500, Marc-André Lureau wrote:
> ----- Original Message -----
> > On Mon, Nov 09, 2015 at 01:34:50PM -0600, Jonathon Jongsma wrote:
> > > On Mon, 2015-11-09 at 12:25 -0500, Marc-André Lureau wrote:
> > > > 
> > > > ----- Original Message -----
> > > > > > 
> > > > > > From: Marc-André Lureau <marcandre.lureau at gmail.com>
> > > > > > 
> > > > > > ---
> > > > > >  server/cursor-channel.c | 6 +++---
> > > > > >  server/red_worker.c     | 2 --
> > > > > >  2 files changed, 3 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > > > > > index 6d648b1..fc3a057 100644
> > > > > > --- a/server/cursor-channel.c
> > > > > > +++ b/server/cursor-channel.c
> > > > > > @@ -219,7 +219,7 @@ static void
> > > > > > put_cursor_pipe_item(CursorChannelClient
> > > > > > *ccc, CursorPipeItem *pipe_
> > > > > >          return;
> > > > > >      }
> > > > > >  
> > > > > > -    spice_assert(!pipe_item_is_linked(&pipe_item->base));
> > > > > > +    spice_return_if_fail(!pipe_item_is_linked(&pipe_item
> > > > > > ->base));
> > > > > >  
> > > > > >      cursor_item_unref(pipe_item->cursor_item);
> > > > > >      free(pipe_item);
> > > > > > @@ -277,7 +277,7 @@ static void
> > > > > > red_marshall_cursor_init(RedChannelClient
> > > > > > *rcc, SpiceMarshaller *bas
> > > > > >      SpiceMsgCursorInit msg;
> > > > > >      AddBufInfo info;
> > > > > >  
> > > > > > -    spice_assert(rcc);
> > > > > > +    spice_return_if_fail(rcc);
> > > > > >      cursor_channel = SPICE_CONTAINEROF(rcc->channel,
> > > > > > CursorChannel,
> > > > > >      common.base);
> > > > > >  
> > > > > >      red_channel_client_init_send_data(rcc,
> > > > > > SPICE_MSG_CURSOR_INIT, NULL);
> > > > > > @@ -410,7 +410,7 @@ static void
> > > > > > cursor_channel_release_item(RedChannelClient
> > > > > > *rcc, PipeItem *item, i
> > > > > >  {
> > > > > >      CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> > > > > >  
> > > > > > -    spice_assert(item);
> > > > > > +    spice_return_if_fail(item);
> > > > > >  
> > > > > >      if (item_pushed) {
> > > > > >          cursor_channel_client_release_item_after_push(ccc,
> > > > > > item);
> > > > > 
> > > > > Considering the default behavior of spice_return_if_fail is
> > > > > aborting
> > > > > like spice_assert I don't see the point of these changes.
> > > > 
> > > > The point is we should have less assert() in the code and be more
> > > > resilient to memory allocation failures or wrong arguments received
> > > > etc. Then the code should no longer assert() in return_if_fail().
> > > 
> > > 
> > > I'm becoming convinced that we should get rid of all of these spice_*
> > > equivalents to the glib logging / checking functions (spice_return_if,
> > > spice_warning, etc). The fact that spice_return_if_fail() aborts but
> > > g_return_if_fail() does not abort creates a potential for
> > > inconsistencies and incorrect usage.
> > 
> > I agree.
> > 
> 
> Sure, you can trivially s/spice_return/g_return once you review all the
> paths. In the meantime, it's better to start using spice_return where it
> makes sense. It will ease a future transition to g_return.

I think it makes sense to write new code with the glib functions for
logging and checking. Patches are being reviewed and would ease
even more the transition after the merge is done.

> 
> > > 
> > > 
> > > > 
> > > > > 
> > > > > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > > > > index f70ee9b..10dfd8b 100644
> > > > > > --- a/server/red_worker.c
> > > > > > +++ b/server/red_worker.c
> > > > > > @@ -301,8 +301,6 @@ struct RedGlzDrawable {
> > > > > >  pthread_mutex_t glz_dictionary_list_lock =
> > > > > > PTHREAD_MUTEX_INITIALIZER;
> > > > > >  Ring glz_dictionary_list = {&glz_dictionary_list,
> > > > > > &glz_dictionary_list};
> > > > > >  
> > > > > > -#define NUM_CURSORS 100
> > > > > > -
> > > > > >  typedef struct RedWorker {
> > > > > >      pthread_t thread;
> > > > > >      clockid_t clockid;
> > > > > > --
> > > > > > 2.4.3
> > > > > > 
> > > > > 
> > > > > I'll split this last hunk in
> > > > > 
> > > > > "worker: remove unused NUM_CURSORS define" (and I'll ack this)
> > > > > 
> > > > > Frediano
> > > > > _______________________________________________
> > > > > Spice-devel mailing list
> > > > > Spice-devel at lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > > > 
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >