[Spice-devel] [spice-server 08/10] Pass RedChannelClient to red_peer_handle_incoming()

[Jonathon Jongsma]

On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> There is only one implementation of IncomingHandler which relies
> IncomingHandler::opaque to be a RedChannlClient. This commit makes
> this
> explicit. This allows to get rid of the IncomingHandler::opaque data
> member.
> 
> If we want to keep some genericity, addressing the FIXME in
> red_channel_client_handle_incoming() would probably allow to move the
> data reading logic to reds-stream.c

Perhaps also worth mentioning that we're also changing the name of this
function from red_peer_handle_incoming() to
red_channel_client_handle_incoming()?

> ---
>  server/red-channel-client-private.h |  1 -
>  server/red-channel-client.c         | 27 ++++++++++++++-------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/server/red-channel-client-private.h b/server/red-
> channel-client-private.h
> index 77766d0..a7167e5 100644
> --- a/server/red-channel-client-private.h
> +++ b/server/red-channel-client-private.h
> @@ -50,7 +50,6 @@ typedef struct OutgoingHandler {
>  } OutgoingHandler;
>  
>  typedef struct IncomingHandler {
> -    void *opaque;
>      uint8_t header_buf[MAX_HEADER_SIZE];
>      SpiceDataHeaderOpaque header;
>      uint32_t header_pos;
> diff --git a/server/red-channel-client.c b/server/red-channel-
> client.c
> index 968d5cd..59f8805 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -268,7 +268,6 @@ static void
> red_channel_client_constructed(GObject *object)
>  {
>      RedChannelClient *self =  RED_CHANNEL_CLIENT(object);
>  
> -    self->priv->incoming.opaque = self;
>      self->priv->outgoing.opaque = self;
>      self->priv->outgoing.pos = 0;
>      self->priv->outgoing.size = 0;
> @@ -1146,8 +1145,10 @@ static int
> red_channel_client_parse(RedChannelClient *client, uint8_t *msg,
>  // does many calls to red_peer_receive and through it cb_read, and
> thus avoids pointer
>  // arithmetic for the case where a single cb_read could return
> multiple messages. But
>  // this is suboptimal potentially. Profile and consider fixing.
> -static void red_peer_handle_incoming(RedsStream *stream,
> IncomingHandler *handler)
> +static void red_channel_client_handle_incoming(RedChannelClient
> *client)

Nitpick (which may apply to other patches as well, but I only just
noticed it here): The vast majority of RedChannelClient methods in this
file name the first argument 'rcc'. There are also a few that call it
'self' and some that call it 'client'. But I think it's nice to try to
be consistent, so perhaps name it 'rcc'?

>  {
> +    RedsStream *stream = client->priv->stream;
> +    IncomingHandler *handler = &client->priv->incoming;
>      int bytes_read;
>      uint16_t msg_type;
>      uint32_t msg_size;
> @@ -1166,11 +1167,11 @@ static void
> red_peer_handle_incoming(RedsStream *stream, IncomingHandler *handle
>                                            handler-
> >header.header_size - handler->header_pos);
>              if (bytes_read == -1) {
>                  /* FIXME: handle disconnection in caller */
> -                red_channel_client_disconnect(handler->opaque);
> +                red_channel_client_disconnect(client);
>                  return;
>              }
>              /* FIXME: return number of bytes read, and emit signal
> from caller */
> -            red_channel_client_on_input(handler->opaque,
> bytes_read);
> +            red_channel_client_on_input(client, bytes_read);
>              handler->header_pos += bytes_read;
>  
>              if (handler->header_pos != handler->header.header_size)
> {
> @@ -1182,11 +1183,11 @@ static void
> red_peer_handle_incoming(RedsStream *stream, IncomingHandler *handle
>          msg_type = handler->header.get_msg_type(&handler->header);
>          if (handler->msg_pos < msg_size) {
>              if (!handler->msg) {
> -                handler->msg =
> red_channel_client_alloc_msg_buf(handler->opaque, msg_type,
> msg_size);
> +                handler->msg =
> red_channel_client_alloc_msg_buf(client, msg_type, msg_size);
>                  if (handler->msg == NULL) {
>                      spice_printerr("ERROR: channel refused to
> allocate buffer.");
>                      /* FIXME: handle disconnection in caller */
> -                    red_channel_client_disconnect(handler->opaque);
> +                    red_channel_client_disconnect(client);
>                      return;
>                  }
>              }
> @@ -1195,26 +1196,26 @@ static void
> red_peer_handle_incoming(RedsStream *stream, IncomingHandler *handle
>                                            handler->msg + handler-
> >msg_pos,
>                                            msg_size - handler-
> >msg_pos);
>              if (bytes_read == -1) {
> -                red_channel_client_release_msg_buf(handler->opaque,
> msg_type, msg_size,
> +                red_channel_client_release_msg_buf(client, msg_type,
> msg_size,
>                                                     handler->msg);
>                  /* FIXME: handle disconnection in caller */
> -                red_channel_client_disconnect(handler->opaque);
> +                red_channel_client_disconnect(client);
>                  return;
>              }
> -            red_channel_client_on_input(handler->opaque,
> bytes_read);
> +            red_channel_client_on_input(client, bytes_read);
>              handler->msg_pos += bytes_read;
>              if (handler->msg_pos != msg_size) {
>                  return;
>              }
>          }
>  
> -        ret_handle = red_channel_client_parse(handler->opaque,
> handler->msg,
> +        ret_handle = red_channel_client_parse(client, handler->msg,
>                                                msg_size, msg_type);
>          if (ret_handle == -1) {
>              return;
>          }
>          handler->msg_pos = 0;
> -        red_channel_client_release_msg_buf(handler->opaque,
> +        red_channel_client_release_msg_buf(client,
>                                             msg_type, msg_size,
>                                             handler->msg);
>          handler->msg = NULL;
> @@ -1222,7 +1223,7 @@ static void red_peer_handle_incoming(RedsStream
> *stream, IncomingHandler *handle
>  
>          if (!ret_handle) {
>              /* FIXME: handle disconnection in caller */
> -            red_channel_client_disconnect(handler->opaque);
> +            red_channel_client_disconnect(client);
>              return;
>          }
>      }
> @@ -1231,7 +1232,7 @@ static void red_peer_handle_incoming(RedsStream
> *stream, IncomingHandler *handle
>  void red_channel_client_receive(RedChannelClient *rcc)
>  {
>      g_object_ref(rcc);
> -    red_peer_handle_incoming(rcc->priv->stream, &rcc->priv-
> >incoming);
> +    red_channel_client_handle_incoming(rcc);
>      g_object_unref(rcc);
>  }
>  


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