[Spice-devel] [PATCH 2/2] Move upcast conversion to a safer place

[Frediano Ziglio]

> 
> On 05/26/2016 02:17 PM, Frediano Ziglio wrote:
> > Upcast conversions are by definition unsafe. There are however some
> > location where such conversion are more safe. In this case send_item
> > callback is registered specifically for this type of RedChannel making
> > the conversion more reliable.
> > The other conversion (CursorChannel -> RedChannel) became safe.
> 
> Hi Frediano,
> 
> I see both channel_send_pipe_item_proc and
> cursor_channel_send_item get rcc parameter.
> 
> I do not follow why this location is safer
> than the rest of the other (static) functions
> in this file.
> 
> Specifically both cursor_marshal and red_marshal_cursor_init
> are only called from cursor_channel_send_item so to me they
> are "as safe".
> 
> Patch itself is ok.
> 
> Thanks,
>      Uri.
> 

I think this patch actually does not change a single byte of
the output, is more theoretical than practical.

The base idea is the locality of the knowledge one must
have. If you look at the single function
(like red_marshal_cursor_init) as a reviewer or somebody
looking for possible problem (like after a core) you don't
have to check the callers to understand that the cast is safe
as being a downcast is safe by definition (yes, can happens
that caller pass a wrong pointer). As small function in a
not so big file is not an issue but just for instance if the
function is moved in another file making it not static then
you'll have to look for all possible caller to make sure that
you can safely do the cast.

For send_item is a bit different as you have to know it's
a callback and take a bit more but if you start from the
marshaller then you find send_item and you have to proceed
back again to understand that is safe instead of stopping
at the first function.

Frediano

> >
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> >  server/cursor-channel.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/server/cursor-channel.c b/server/cursor-channel.c
> > index 6a0ebff..d8d3e27 100644
> > --- a/server/cursor-channel.c
> > +++ b/server/cursor-channel.c
> > @@ -226,11 +226,11 @@ static void
> > cursor_channel_client_on_disconnect(RedChannelClient *rcc)
> >      red_reset_cursor_cache(rcc);
> >  }
> >
> > -static void red_marshall_cursor_init(RedChannelClient *rcc,
> > SpiceMarshaller *base_marshaller,
> > +static void red_marshall_cursor_init(CursorChannelClient *ccc,
> > SpiceMarshaller *base_marshaller,
> >                                       RedPipeItem *pipe_item)
> >  {
> >      CursorChannel *cursor_channel;
> > -    CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> > +    RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
> >      SpiceMsgCursorInit msg;
> >      AddBufInfo info;
> >
> > @@ -248,12 +248,12 @@ static void red_marshall_cursor_init(RedChannelClient
> > *rcc, SpiceMarshaller *bas
> >      add_buf_from_info(base_marshaller, &info);
> >  }
> >
> > -static void cursor_marshall(RedChannelClient *rcc,
> > +static void cursor_marshall(CursorChannelClient *ccc,
> >                              SpiceMarshaller *m,
> >                              RedCursorPipeItem *cursor_pipe_item)
> >  {
> > +    RedChannelClient *rcc = RED_CHANNEL_CLIENT(ccc);
> >      CursorChannel *cursor_channel = SPICE_CONTAINEROF(rcc->channel,
> >      CursorChannel, common.base);
> > -    CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> >      CursorItem *item = cursor_pipe_item->cursor_item;
> >      RedPipeItem *pipe_item = &cursor_pipe_item->base;
> >      RedCursorCmd *cmd;
> > @@ -317,10 +317,11 @@ static inline void
> > red_marshall_inval(RedChannelClient *rcc,
> >  static void cursor_channel_send_item(RedChannelClient *rcc, RedPipeItem
> >  *pipe_item)
> >  {
> >      SpiceMarshaller *m = red_channel_client_get_marshaller(rcc);
> > +    CursorChannelClient *ccc = RCC_TO_CCC(rcc);
> >
> >      switch (pipe_item->type) {
> >      case RED_PIPE_ITEM_TYPE_CURSOR:
> > -        cursor_marshall(rcc, m, SPICE_UPCAST(RedCursorPipeItem,
> > pipe_item));
> > +        cursor_marshall(ccc, m, SPICE_UPCAST(RedCursorPipeItem,
> > pipe_item));
> >          break;
> >      case RED_PIPE_ITEM_TYPE_INVAL_ONE:
> >          red_marshall_inval(rcc, m, SPICE_CONTAINEROF(pipe_item,
> >          RedCacheItem, u.pipe_data));
> > @@ -330,7 +331,7 @@ static void cursor_channel_send_item(RedChannelClient
> > *rcc, RedPipeItem *pipe_it
> >          break;
> >      case RED_PIPE_ITEM_TYPE_CURSOR_INIT:
> >          red_reset_cursor_cache(rcc);
> > -        red_marshall_cursor_init(rcc, m, pipe_item);
> > +        red_marshall_cursor_init(ccc, m, pipe_item);
> >          break;
> >      case RED_PIPE_ITEM_TYPE_INVAL_CURSOR_CACHE:
> >          red_reset_cursor_cache(rcc);
> >
> 
>