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

[Jonathon Jongsma]

On Thu, 2016-05-26 at 14:12 -0400, Frediano Ziglio wrote:
> > 
> > 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.

Agreed

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>

> 
> 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);
> > > 
> > 
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel