[Spice-devel] [PATCH 02/24] server/red_channel: add hold_item (from red_worker)

Alon Levy alevy at redhat.com
Mon Feb 7 05:26:40 PST 2011


On Mon, Feb 07, 2011 at 03:14:36PM +0200, Alon Levy wrote:
> On Wed, Feb 02, 2011 at 07:49:43PM +0100, Marc-André Lureau wrote:
> > On Wed, Jan 19, 2011 at 7:07 PM, Alon Levy <alevy at redhat.com> wrote:
> > > hold_item called on init_send_data, matching release.
> > > This is not the behavior of red_worker - we ref++ (==hold_item) when
> > > sending the item, and --refs when releasing it, instead of only holding
> > > if the send is blocked.
> > > main, inputs, tunnel and smartcard channels have empty implementation.
> > 
> > Since we have release_pipe_item,  s / hold_item / hold_pipe_item ?
> 
> Sounds good.

I'm changing the function signature, but since the variable naming has been
release_item (not release_pipe_item) I'm keeping them as hold_item.

> 
> > 
> > So who has an implementation of this cb in the end?
> 
> display_channel and cursor_channel will once the refactor is done.
> 
> > 
> > > ---
> > >  server/inputs_channel.c    |    5 +++++
> > >  server/main_channel.c      |    5 +++++
> > >  server/red_channel.c       |   12 ++++++++++--
> > >  server/red_channel.h       |    4 ++++
> > >  server/red_tunnel_worker.c |    5 +++++
> > >  server/smartcard.c         |    5 +++++
> > >  6 files changed, 34 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/server/inputs_channel.c b/server/inputs_channel.c
> > > index b7ae55a..f9777ed 100644
> > > --- a/server/inputs_channel.c
> > > +++ b/server/inputs_channel.c
> > > @@ -508,6 +508,10 @@ static int inputs_channel_config_socket(RedChannel *channel)
> > >     return TRUE;
> > >  }
> > >
> > > +static void inputs_channel_hold_item(PipeItem *item)
> > > +{
> > > +}
> > > +
> > >  static void inputs_link(Channel *channel, RedsStreamContext *peer, int migration,
> > >                         int num_common_caps, uint32_t *common_caps, int num_caps,
> > >                         uint32_t *caps)
> > > @@ -523,6 +527,7 @@ static void inputs_link(Channel *channel, RedsStreamContext *peer, int migration
> > >         ,inputs_channel_handle_parsed
> > >         ,inputs_channel_alloc_msg_rcv_buf
> > >         ,inputs_channel_release_msg_rcv_buf
> > > +        ,inputs_channel_hold_item
> > >         ,inputs_channel_send_item
> > >         ,inputs_channel_release_pipe_item
> > >         ,inputs_channel_on_incoming_error
> > > diff --git a/server/main_channel.c b/server/main_channel.c
> > > index f1fb4c6..72ba460 100644
> > > --- a/server/main_channel.c
> > > +++ b/server/main_channel.c
> > > @@ -776,6 +776,10 @@ static int main_channel_config_socket(RedChannel *channel)
> > >     return TRUE;
> > >  }
> > >
> > > +static void main_channel_hold_item(PipeItem *item)
> > > +{
> > > +}
> > > +
> > >  static void main_channel_link(Channel *channel, RedsStreamContext *peer, int migration,
> > >                         int num_common_caps, uint32_t *common_caps, int num_caps,
> > >                         uint32_t *caps)
> > > @@ -791,6 +795,7 @@ static void main_channel_link(Channel *channel, RedsStreamContext *peer, int mig
> > >         ,main_channel_handle_parsed
> > >         ,main_channel_alloc_msg_rcv_buf
> > >         ,main_channel_release_msg_rcv_buf
> > > +        ,main_channel_hold_item
> > >         ,main_channel_send_item
> > >         ,main_channel_release_pipe_item
> > >         ,main_channel_on_error
> > > diff --git a/server/red_channel.c b/server/red_channel.c
> > > index 584b92e..c208ec3 100644
> > > --- a/server/red_channel.c
> > > +++ b/server/red_channel.c
> > > @@ -224,6 +224,7 @@ static void red_channel_peer_prepare_out_msg(void *opaque, struct iovec *vec, in
> > >  static void red_channel_peer_on_out_block(void *opaque)
> > >  {
> > >     RedChannel *channel = (RedChannel *)opaque;
> > > +
> > >     channel->send_data.blocked = TRUE;
> > >     channel->core->watch_update_mask(channel->peer->watch,
> > >                                      SPICE_WATCH_EVENT_READ |
> > > @@ -253,6 +254,7 @@ RedChannel *red_channel_create(int size, RedsStreamContext *peer,
> > >                                channel_handle_message_proc handle_message,
> > >                                channel_alloc_msg_recv_buf_proc alloc_recv_buf,
> > >                                channel_release_msg_recv_buf_proc release_recv_buf,
> > > +                               channel_hold_item_proc hold_item,
> > >                                channel_send_pipe_item_proc send_item,
> > >                                channel_release_pipe_item_proc release_item)
> > >  {
> > > @@ -267,6 +269,7 @@ RedChannel *red_channel_create(int size, RedsStreamContext *peer,
> > >     channel->disconnect = disconnect;
> > >     channel->send_item = send_item;
> > >     channel->release_item = release_item;
> > > +    channel->hold_item = hold_item;
> > >
> > >     channel->peer = peer;
> > >     channel->core = core;
> > > @@ -332,6 +335,7 @@ RedChannel *red_channel_create_parser(int size, RedsStreamContext *peer,
> > >                                channel_handle_parsed_proc handle_parsed,
> > >                                channel_alloc_msg_recv_buf_proc alloc_recv_buf,
> > >                                channel_release_msg_recv_buf_proc release_recv_buf,
> > > +                               channel_hold_item_proc hold_item,
> > >                                channel_send_pipe_item_proc send_item,
> > >                                channel_release_pipe_item_proc release_item,
> > >                                channel_on_incoming_error_proc incoming_error,
> > > @@ -339,7 +343,7 @@ RedChannel *red_channel_create_parser(int size, RedsStreamContext *peer,
> > >  {
> > >     RedChannel *channel = red_channel_create(size, peer,
> > >         core, migrate, handle_acks, config_socket, do_nothing_disconnect, do_nothing_handle_message,
> > > -        alloc_recv_buf, release_recv_buf, send_item, release_item);
> > > +        alloc_recv_buf, release_recv_buf, hold_item, send_item, release_item);
> > >
> > >     if (channel == NULL) {
> > >         return NULL;
> > > @@ -439,7 +443,11 @@ void red_channel_reset_send_data(RedChannel *channel)
> > >  void red_channel_init_send_data(RedChannel *channel, uint16_t msg_type, PipeItem *item)
> > >  {
> > >     channel->send_data.header->type = msg_type;
> > > -    channel->send_data.item = item;
> > > +    if (item) {
> > > +        ASSERT(channel->send_data.item == NULL);
> > > +        channel->send_data.item = item;
> > > +        channel->hold_item(item);
> > > +    }
> > >  }
> > >
> > 
> > As discussed on irc,
> > please void usage of ASSERT(). It can survive this condition/leak, I
> > would prefer a critical log.
> > 
> > Also, instead of empty function implementation, could we have a check such as:
> > 
> > if (channel->hold_item)
> >   channel->hold_item(item);
> > 
> > (I think this is common practice in glib/gobject with vmethods, I
> > might be biased)
> > 
> > >  static void red_channel_send(RedChannel *channel)
> > > diff --git a/server/red_channel.h b/server/red_channel.h
> > > index e8ebb05..db0ae9a 100644
> > > --- a/server/red_channel.h
> > > +++ b/server/red_channel.h
> > > @@ -107,6 +107,7 @@ typedef void (*channel_release_msg_recv_buf_proc)(RedChannel *channel,
> > >  typedef void (*channel_disconnect_proc)(RedChannel *channel);
> > >  typedef int (*channel_configure_socket_proc)(RedChannel *channel);
> > >  typedef void (*channel_send_pipe_item_proc)(RedChannel *channel, PipeItem *item);
> > > +typedef void (*channel_hold_item_proc)(PipeItem *item);
> > >  typedef void (*channel_release_pipe_item_proc)(RedChannel *channel,
> > >                                                PipeItem *item, int item_pushed);
> > >  typedef void (*channel_on_incoming_error_proc)(RedChannel *channel);
> > > @@ -145,6 +146,7 @@ struct RedChannel {
> > >
> > >     channel_disconnect_proc disconnect;
> > >     channel_send_pipe_item_proc send_item;
> > > +    channel_hold_item_proc hold_item;
> > >     channel_release_pipe_item_proc release_item;
> > >
> > >     int during_send;
> > > @@ -165,6 +167,7 @@ RedChannel *red_channel_create(int size, RedsStreamContext *peer,
> > >                                channel_handle_message_proc handle_message,
> > >                                channel_alloc_msg_recv_buf_proc alloc_recv_buf,
> > >                                channel_release_msg_recv_buf_proc release_recv_buf,
> > > +                               channel_hold_item_proc hold_item,
> > >                                channel_send_pipe_item_proc send_item,
> > >                                channel_release_pipe_item_proc release_item);
> > >
> > > @@ -178,6 +181,7 @@ RedChannel *red_channel_create_parser(int size, RedsStreamContext *peer,
> > >                                channel_handle_parsed_proc handle_parsed,
> > >                                channel_alloc_msg_recv_buf_proc alloc_recv_buf,
> > >                                channel_release_msg_recv_buf_proc release_recv_buf,
> > > +                               channel_hold_item_proc hold_item,
> > >                                channel_send_pipe_item_proc send_item,
> > >                                channel_release_pipe_item_proc release_item,
> > >                                channel_on_incoming_error_proc incoming_error,
> > > diff --git a/server/red_tunnel_worker.c b/server/red_tunnel_worker.c
> > > index 6092a76..6d6cc7a 100644
> > > --- a/server/red_tunnel_worker.c
> > > +++ b/server/red_tunnel_worker.c
> > > @@ -3420,6 +3420,10 @@ static void on_new_tunnel_channel(TunnelChannel *channel)
> > >     }
> > >  }
> > >
> > > +static void tunnel_channel_hold_item(PipeItem *item)
> > > +{
> > > +}
> > > +
> > >  static void handle_tunnel_channel_link(Channel *channel, RedsStreamContext *peer, int migration,
> > >                                        int num_common_caps, uint32_t *common_caps, int num_caps,
> > >                                        uint32_t *caps)
> > > @@ -3438,6 +3442,7 @@ static void handle_tunnel_channel_link(Channel *channel, RedsStreamContext *peer
> > >                                             tunnel_channel_handle_message,
> > >                                             tunnel_channel_alloc_msg_rcv_buf,
> > >                                             tunnel_channel_release_msg_rcv_buf,
> > > +                                            tunnel_channel_hold_item,
> > >                                             tunnel_channel_send_item,
> > >                                             tunnel_channel_release_pipe_item);
> > >
> > > diff --git a/server/smartcard.c b/server/smartcard.c
> > > index 7c0a5aa..318cb27 100644
> > > --- a/server/smartcard.c
> > > +++ b/server/smartcard.c
> > > @@ -485,6 +485,10 @@ static int smartcard_channel_handle_message(RedChannel *channel, SpiceDataHeader
> > >     return TRUE;
> > >  }
> > >
> > > +static void smartcard_channel_hold_item(PipeItem *item)
> > > +{
> > > +}
> > > +
> > >  static void smartcard_link(Channel *channel, RedsStreamContext *peer,
> > >                         int migration, int num_common_caps,
> > >                         uint32_t *common_caps, int num_caps,
> > > @@ -502,6 +506,7 @@ static void smartcard_link(Channel *channel, RedsStreamContext *peer,
> > >                                         smartcard_channel_handle_message,
> > >                                         smartcard_channel_alloc_msg_rcv_buf,
> > >                                         smartcard_channel_release_msg_rcv_buf,
> > > +                                        smartcard_channel_hold_item,
> > >                                         smartcard_channel_send_item,
> > >                                         smartcard_channel_release_pipe_item);
> > >     if (!g_smartcard_channel) {
> > > --
> > > 1.7.3.4
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > 
> > 
> > 
> > -- 
> > Marc-André Lureau
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel


More information about the Spice-devel mailing list