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@xxxxxxxxxx>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel