[Spice-devel] [PATCH] Avoid to access data before a NULL check

[Jonathon Jongsma]

On Wed, 2016-05-25 at 13:02 +0200, Christophe Fergeau wrote:
> On Wed, May 25, 2016 at 11:31:13AM +0100, Frediano Ziglio wrote:
> > If you are testing for NULL data this means that variable could be
> > NULL so avoid to access before the check to make sure the check is hit.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/cursor-channel.c  |  5 +++--
> >  server/dcc-send.c        |  6 ++++--
> >  server/dcc.c             |  2 +-
> >  server/display-channel.c |  6 +++---
> >  server/sound.c           | 24 +++++++++++++-----------
> >  server/stream.c          |  3 ++-
> >  6 files changed, 26 insertions(+), 20 deletions(-)
> > 
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index ab6864c..e91aac8 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -227,12 +227,13 @@ static void
> > cursor_channel_client_on_disconnect(RedChannelClient *rcc)
> >  static void red_marshall_cursor_init(RedChannelClient *rcc, SpiceMarshaller
> > *base_marshaller,
> >                                       RedPipeItem *pipe_item)
> >  {
> > +    spice_assert(rcc);
> > +
> >      CursorChannel *cursor_channel;
> >      CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> >      SpiceMsgCursorInit msg;
> >      AddBufInfo info;
> >  
> > -    spice_assert(rcc);
> 
> I'd prefer
> 
> CursorChannelClient *ccc;
> 
> spice_assert(rcc);
> ccc = RCC_TO_CCC(rcc);
> 
> ie keep the declaration grouped at the beginning of the block.
> 
> An additional comment is that some of the changes in here are candidates
> for SPICE_UPCAST, which would make sense to be NULL-safe, so the code
> could stay as is.
> 
> I was also wondering why we have macros with cryptic names rather than
> actual accessors when doing non-upcast? Ie why DCC_TO_WORKER/DCC_TO_DC
> rather than red_display_channel_client_get_channel()? (or even
> red_channel_client_get_channel() if this makes sense inheritance-wise)

I guess just for historical reasons. red_channel_client_get_channel() is
introduced in a patch in the refactory branch that hasn't been merged yet.

Jonathon