[Spice-devel] [spice-server 07/10] Remove IncomingHandlerInterface
Jonathon Jongsma
jjongsma at redhat.com
Thu Feb 9 17:33:41 UTC 2017
On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> This commit removes what remains of IncomingHandlerInterface. The
> remaining function pointers were pointing to RedChannel vfuncs. We
> can
> dereference these directly rather than going through a 2nd layer of
> indirection.
> ---
> server/red-channel-client-private.h | 1 -
> server/red-channel-client.c | 60 +++++++++++++++++++++----
> ------------
> server/red-channel.c | 11 -------
> server/red-channel.h | 11 -------
> 4 files changed, 35 insertions(+), 48 deletions(-)
>
> diff --git a/server/red-channel-client-private.h b/server/red-
> channel-client-private.h
> index 5d29f32..77766d0 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 {
> - IncomingHandlerInterface *cb;
> void *opaque;
> uint8_t header_buf[MAX_HEADER_SIZE];
> SpiceDataHeaderOpaque header;
Same comment as for the OutgoingHandler struct: removing these callback
funcs means that the IncomingHandler name is no longer a very good fit.
It's not so much a 'handler' as it is a collection of memory buffers
and state variables for the incoming message. Perhaps a rename would
increase understandability?
> diff --git a/server/red-channel-client.c b/server/red-channel-
> client.c
> index e93ef4b..968d5cd 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -269,8 +269,6 @@ static void
> red_channel_client_constructed(GObject *object)
> RedChannelClient *self = RED_CHANNEL_CLIENT(object);
>
> self->priv->incoming.opaque = self;
> - self->priv->incoming.cb = red_channel_get_incoming_handler(self-
> >priv->channel);
> -
> self->priv->outgoing.opaque = self;
> self->priv->outgoing.pos = 0;
> self->priv->outgoing.size = 0;
> @@ -1113,6 +1111,37 @@ static int red_peer_receive(RedsStream
> *stream, uint8_t *buf, uint32_t size)
> return pos - buf;
> }
>
> +static int red_channel_client_parse(RedChannelClient *client,
> uint8_t *msg,
> + uint32_t msg_size, uint16_t
> msg_type)
> +{
> + RedChannel *channel = red_channel_client_get_channel(client);
> + RedChannelClass *klass = RED_CHANNEL_GET_CLASS(channel);
> + int ret_handle;
> +
> + if (klass->parser) {
> + uint8_t *parsed;
> + size_t parsed_size;
> + message_destructor_t parsed_free;
> +
> + parsed = klass->parser(msg, msg + msg_size, msg_type,
> + SPICE_VERSION_MINOR, &parsed_size,
> &parsed_free);
> + if (parsed == NULL) {
> + spice_printerr("failed to parse message type %d",
> msg_type);
> + red_channel_client_release_msg_buf(client, msg_type,
> msg_size, msg);
> + /* FIXME: handle disconnection in caller */
> + red_channel_client_disconnect(client);
> + return -1;
I'm not sure about this. Our handle_parsed()/handle_message vfuncs
technically have an 'int' return value, but we treat them as booleans
(most vfunc implementations return TRUE or FALSE). Failures in those
functions are represented by a 0 (FALSE). This new function will
sometimes return those values directly, but sometimes return a special
-1. So we have the following possibilities: -1 (failure), 0 (failures,
or 1 (success). It works, but I find it a little odd. And it confused
me a bit when reviewing the code below (see comment below).
> + }
> + ret_handle = klass->handle_parsed(client, parsed_size,
> + msg_type, parsed);
> + parsed_free(parsed);
> + } else {
> + ret_handle = klass->handle_message(client, msg_type,
> msg_size, msg);
> + }
> +
> + return ret_handle;
> +}
> +
> // TODO: this implementation, as opposed to the old implementation
> in red_worker,
> // 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
> @@ -1179,29 +1208,10 @@ static void
> red_peer_handle_incoming(RedsStream *stream, IncomingHandler *handle
> }
> }
>
> - if (handler->cb->parser) {
> - uint8_t *parsed;
> - size_t parsed_size;
> - message_destructor_t parsed_free;
> -
> - parsed = handler->cb->parser(handler->msg,
> - handler->msg + msg_size, msg_type,
> - SPICE_VERSION_MINOR, &parsed_size, &parsed_free);
> - if (parsed == NULL) {
> - spice_printerr("failed to parse message type %d",
> msg_type);
> - red_channel_client_release_msg_buf(handler->opaque,
> - msg_type,
> msg_size,
> - handler->msg);
> - /* FIXME: handle disconnection in caller */
> - red_channel_client_disconnect(handler->opaque);
> - return;
> - }
> - ret_handle = handler->cb->handle_parsed(handler->opaque,
> parsed_size,
> - msg_type,
> parsed);
> - parsed_free(parsed);
> - } else {
> - ret_handle = handler->cb->handle_message(handler-
> >opaque, msg_type, msg_size,
> - handler->msg);
> + ret_handle = red_channel_client_parse(handler->opaque,
> handler->msg,
> + msg_size, msg_type);
> + if (ret_handle == -1) {
> + return;
> }
(see above) I initially thought you were changing behavior here because
you were returning from this function on failure, whereas before we
would not return if e.g. handle_message() failed. But then I noticed
that -1 is a special failure value, and
handle_message()/handle_parsed() returns a different failure value
(FALSE). So it does appear to maintain the same behavior as before, but
as I mentioned above, I find it slightly confusing to have these two
different failure values, only one of which evaluates to FALSE.
> handler->msg_pos = 0;
> red_channel_client_release_msg_buf(handler->opaque,
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 0c809b6..0f73c7e 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -91,8 +91,6 @@ struct RedChannelPrivate
> RedChannelCapabilities local_caps;
> uint32_t migration_flags;
>
> - IncomingHandlerInterface incoming_cb;
> -
> ClientCbs client_cbs;
> // TODO: when different channel_clients are in different threads
> // from Channel -> need to protect!
> @@ -218,10 +216,6 @@ red_channel_constructed(GObject *object)
> klass->alloc_recv_buf && klass->release_recv_buf);
> spice_assert(klass->handle_migrate_data ||
> !(self->priv->migration_flags &
> SPICE_MIGRATE_NEED_DATA_TRANSFER));
> -
> - self->priv->incoming_cb.handle_message =
> (handle_message_proc)klass->handle_message;
> - self->priv->incoming_cb.handle_parsed =
> (handle_parsed_proc)klass->handle_parsed;
> - self->priv->incoming_cb.parser = klass->parser;
> }
>
> static void red_channel_client_default_connect(RedChannel *channel,
> RedClient *client,
> @@ -761,11 +755,6 @@ void red_channel_send_item(RedChannel *self,
> RedChannelClient *rcc, RedPipeItem
> klass->send_item(rcc, item);
> }
>
> -IncomingHandlerInterface*
> red_channel_get_incoming_handler(RedChannel *self)
> -{
> - return &self->priv->incoming_cb;
> -}
> -
> void red_channel_reset_thread_id(RedChannel *self)
> {
> self->priv->thread_id = pthread_self();
> diff --git a/server/red-channel.h b/server/red-channel.h
> index e7c1e1f..0385595 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -68,13 +68,6 @@ typedef void (*release_msg_recv_buf_proc)(void
> *opaque,
> typedef void (*on_incoming_error_proc)(void *opaque);
> typedef void (*on_input_proc)(void *opaque, int n);
>
> -typedef struct IncomingHandlerInterface {
> - handle_message_proc handle_message;
> - // The following is an optional alternative to handle_message,
> used if not null
> - spice_parse_channel_func_t parser;
> - handle_parsed_proc handle_parsed;
> -} IncomingHandlerInterface;
> -
> typedef struct RedChannel RedChannel;
> typedef struct RedChannelClient RedChannelClient;
> typedef struct RedClient RedClient;
> @@ -283,10 +276,6 @@ void red_channel_send_item(RedChannel *self,
> RedChannelClient *rcc, RedPipeItem
> void red_channel_reset_thread_id(RedChannel *self);
> StatNodeRef red_channel_get_stat_node(RedChannel *channel);
>
> -/* FIXME: does this even need to be in RedChannel? It's really only
> used in
> - * RedChannelClient. Needs refactoring */
> -IncomingHandlerInterface*
> red_channel_get_incoming_handler(RedChannel *self);
> -
> const RedChannelCapabilities*
> red_channel_get_local_capabilities(RedChannel *self);
>
> /*
More information about the Spice-devel
mailing list