Re: [spice-server 07/10] Remove IncomingHandlerInterface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]