On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> RedChannel uses OutgoingHandlerInterface to provide constant pointers
> to
> RedChannelClient methods. This OutgoingHandlerInterface structure is
> then used in RedChannelClient to indirectly call these methods.
>
> Since the content of OutgoingHandlerInterface is never modified, we
> can
> directly call the relevant methods and make them static.
> ---
> server/red-channel-client-private.h | 1 -
> server/red-channel-client.c | 30 ++++++++++++++++-----------
> ---
> server/red-channel-client.h | 6 ------
> server/red-channel.c | 13 -------------
> server/red-channel.h | 20 +-------------------
> 5 files changed, 17 insertions(+), 53 deletions(-)
>
> diff --git a/server/red-channel-client-private.h b/server/red-
> channel-client-private.h
> index d01cdbd..5d29f32 100644
> --- a/server/red-channel-client-private.h
> +++ b/server/red-channel-client-private.h
> @@ -41,7 +41,6 @@ typedef struct RedChannelClientConnectivityMonitor
> {
> } RedChannelClientConnectivityMonitor;
>
> typedef struct OutgoingHandler {
> - OutgoingHandlerInterface *cb;
> void *opaque;
> struct iovec vec_buf[IOV_MAX];
> int vec_size;
So, after removing the 'cb' field here, we're basically left with some
buffers. In my mind, that makes OutgoingHandler a bit of a misnomer.
Perhaps we could rename it to something that describes its function a
bit better?
> diff --git a/server/red-channel-client.c b/server/red-channel-
> client.c
> index 82c786f..a5c8e9f 100644
> --- a/server/red-channel-client.c
> +++ b/server/red-channel-client.c
> @@ -272,7 +272,6 @@ static void
> red_channel_client_constructed(GObject *object)
> self->priv->incoming.cb = red_channel_get_incoming_handler(self-
> >priv->channel);
>
> self->priv->outgoing.opaque = self;
> - self->priv->outgoing.cb = red_channel_get_outgoing_handler(self-
> >priv->channel);
> self->priv->outgoing.pos = 0;
> self->priv->outgoing.size = 0;
>
> @@ -373,7 +372,7 @@ RedChannel*
> red_channel_client_get_channel(RedChannelClient *rcc)
> return rcc->priv->channel;
> }
>
> -void red_channel_client_on_output(void *opaque, int n)
> +static void red_channel_client_on_output(void *opaque, int n)
> {
> RedChannelClient *rcc = opaque;
> RedChannel *channel = red_channel_client_get_channel(rcc);
> @@ -381,6 +380,7 @@ void red_channel_client_on_output(void *opaque,
> int n)
> if (rcc->priv->connectivity_monitor.timer) {
> rcc->priv->connectivity_monitor.out_bytes += n;
> }
> + /* FIXME: use a signal rather than a hardcoded call to a
> RedChannel callback? */
That sounds nicer to me. That would remove the need to expose this from
the header (as I mentioned in the review for the last patch)
> red_channel_on_output(channel, n);
> }
>
> @@ -393,15 +393,15 @@ void red_channel_client_on_input(void *opaque,
> int n)
> }
> }
>
> -int red_channel_client_get_out_msg_size(void *opaque)
> +static int red_channel_client_get_out_msg_size(void *opaque)
> {
> RedChannelClient *rcc = RED_CHANNEL_CLIENT(opaque);
>
> return rcc->priv->send_data.size;
> }
>
> -void red_channel_client_prepare_out_msg(void *opaque, struct iovec
> *vec,
> - int *vec_size, int pos)
> +static void red_channel_client_prepare_out_msg(void *opaque, struct
> iovec *vec,
> + int *vec_size, int
> pos)
> {
> RedChannelClient *rcc = RED_CHANNEL_CLIENT(opaque);
>
> @@ -409,7 +409,7 @@ void red_channel_client_prepare_out_msg(void
> *opaque, struct iovec *vec,
> vec, IOV_MAX, pos);
> }
>
> -void red_channel_client_on_out_block(void *opaque)
> +static void red_channel_client_on_out_block(void *opaque)
> {
> SpiceCoreInterfaceInternal *core;
> RedChannelClient *rcc = RED_CHANNEL_CLIENT(opaque);
> @@ -549,7 +549,7 @@ static void
> red_channel_client_restore_main_sender(RedChannelClient *rcc)
> rcc->priv->send_data.header.data = rcc->priv-
> >send_data.main.header_data;
> }
>
> -void red_channel_client_on_out_msg_done(void *opaque)
> +static void red_channel_client_on_out_msg_done(void *opaque)
> {
> RedChannelClient *rcc = RED_CHANNEL_CLIENT(opaque);
> int fd;
> @@ -1010,33 +1010,35 @@ static void
> red_peer_handle_outgoing(RedsStream *stream, OutgoingHandler *handle
>
> if (handler->size == 0) {
> handler->vec = handler->vec_buf;
> - handler->size = handler->cb->get_msg_size(handler->opaque);
> + handler->size = red_channel_client_get_out_msg_size(handler-
> >opaque);
> if (!handler->size) { // nothing to be sent
> return;
> }
> }
>
> for (;;) {
> - handler->cb->prepare(handler->opaque, handler->vec,
> &handler->vec_size, handler->pos);
> + red_channel_client_prepare_out_msg(handler->opaque, handler-
> >vec, &handler->vec_size, handler->pos);
> n = reds_stream_writev(stream, handler->vec, handler-
> >vec_size);
> if (n == -1) {
> switch (errno) {
> case EAGAIN:
> - handler->cb->on_block(handler->opaque);
> + red_channel_client_on_out_block(handler->opaque);
> return;
> case EINTR:
> continue;
> case EPIPE:
> - handler->cb->on_error(handler->opaque);
> + /* FIXME: handle disconnection in caller */
> + red_channel_client_disconnect(handler->opaque);
> return;
> default:
> + /* FIXME: handle disconnection in caller */
> spice_printerr("%s", strerror(errno));
> - handler->cb->on_error(handler->opaque);
> + red_channel_client_disconnect(handler->opaque);
> return;
> }
> } else {
> handler->pos += n;
> - handler->cb->on_output(handler->opaque, n);
> + red_channel_client_on_output(handler->opaque, n);
> if (handler->pos == handler->size) { // finished writing
> data
> /* reset handler before calling on_msg_done, since
> it
> * can trigger another call to
> red_peer_handle_outgoing (when
> @@ -1044,7 +1046,7 @@ static void red_peer_handle_outgoing(RedsStream
> *stream, OutgoingHandler *handle
> handler->vec = handler->vec_buf;
> handler->pos = 0;
> handler->size = 0;
> - handler->cb->on_msg_done(handler->opaque);
> + red_channel_client_on_out_msg_done(handler->opaque);
> return;
> }
> }
> diff --git a/server/red-channel-client.h b/server/red-channel-
> client.h
> index fada609..6b6a61a 100644
> --- a/server/red-channel-client.h
> +++ b/server/red-channel-client.h
> @@ -175,13 +175,7 @@ void
> red_channel_client_disconnect_if_pending_send(RedChannelClient *rcc);
>
> RedChannel* red_channel_client_get_channel(RedChannelClient *rcc);
>
> -void red_channel_client_on_output(void *opaque, int n);
> void red_channel_client_on_input(void *opaque, int n);
> -int red_channel_client_get_out_msg_size(void *opaque);
> -void red_channel_client_prepare_out_msg(void *opaque, struct iovec
> *vec,
> - int *vec_size, int
> pos);
> -void red_channel_client_on_out_block(void *opaque);
> -void red_channel_client_on_out_msg_done(void *opaque);
>
> void
> red_channel_client_semi_seamless_migration_complete(RedChannelClient
> *rcc);
> void
> red_channel_client_init_outgoing_messages_window(RedChannelClient
> *rcc);
> diff --git a/server/red-channel.c b/server/red-channel.c
> index 92a22de..3abe9c7 100644
> --- a/server/red-channel.c
> +++ b/server/red-channel.c
> @@ -91,7 +91,6 @@ struct RedChannelPrivate
> RedChannelCapabilities local_caps;
> uint32_t migration_flags;
>
> - OutgoingHandlerInterface outgoing_cb;
> IncomingHandlerInterface incoming_cb;
>
> ClientCbs client_cbs;
> @@ -333,13 +332,6 @@ red_channel_init(RedChannel *self)
> self->priv->incoming_cb.on_error =
> (on_incoming_error_proc)red_channel_client_default_peer_on_e
> rror;
> self->priv->incoming_cb.on_input = red_channel_client_on_input;
> - self->priv->outgoing_cb.get_msg_size =
> red_channel_client_get_out_msg_size;
> - self->priv->outgoing_cb.prepare =
> red_channel_client_prepare_out_msg;
> - self->priv->outgoing_cb.on_block =
> red_channel_client_on_out_block;
> - self->priv->outgoing_cb.on_error =
> - (on_outgoing_error_proc)red_channel_client_default_peer_on_e
> rror;
> - self->priv->outgoing_cb.on_msg_done =
> red_channel_client_on_out_msg_done;
> - self->priv->outgoing_cb.on_output =
> red_channel_client_on_output;
>
> self->priv->client_cbs.connect =
> red_channel_client_default_connect;
> self->priv->client_cbs.disconnect =
> red_channel_client_default_disconnect;
> @@ -788,11 +780,6 @@ IncomingHandlerInterface*
> red_channel_get_incoming_handler(RedChannel *self)
> return &self->priv->incoming_cb;
> }
>
> -OutgoingHandlerInterface*
> red_channel_get_outgoing_handler(RedChannel *self)
> -{
> - return &self->priv->outgoing_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 291a00f..a24330a 100644
> --- a/server/red-channel.h
> +++ b/server/red-channel.h
> @@ -79,23 +79,6 @@ typedef struct IncomingHandlerInterface {
> on_input_proc on_input;
> } IncomingHandlerInterface;
>
> -typedef int (*get_outgoing_msg_size_proc)(void *opaque);
> -typedef void (*prepare_outgoing_proc)(void *opaque, struct iovec
> *vec, int *vec_size, int pos);
> -typedef void (*on_outgoing_error_proc)(void *opaque);
> -typedef void (*on_outgoing_block_proc)(void *opaque);
> -typedef void (*on_outgoing_msg_done_proc)(void *opaque);
> -typedef void (*on_output_proc)(void *opaque, int n);
> -
> -typedef struct OutgoingHandlerInterface {
> - get_outgoing_msg_size_proc get_msg_size;
> - prepare_outgoing_proc prepare;
> - on_outgoing_error_proc on_error;
> - on_outgoing_block_proc on_block;
> - on_outgoing_msg_done_proc on_msg_done;
> - on_output_proc on_output;
> -} OutgoingHandlerInterface;
> -/* Red Channel interface */
> -
> typedef struct RedChannel RedChannel;
> typedef struct RedChannelClient RedChannelClient;
> typedef struct RedClient RedClient;
> @@ -304,10 +287,9 @@ 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: do these even need to be in RedChannel? It's really only
> used in
> +/* 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);
> -OutgoingHandlerInterface*
> red_channel_get_outgoing_handler(RedChannel *self);
>
> const RedChannelCapabilities*
> red_channel_get_local_capabilities(RedChannel *self);
>
Not directly related to this patch, other than the fact that the patch
made me start looking into stuff a bit closer: I noticed in looking
through red_channel_client_prepare_out_msg() that we call
spice_marshaller_fill_iovec() with a hard-coded n_vec argument of
IOV_MAX. This works fine right now because OutgoingHandler::vec_buf is
defined with a size of IOV_MAX. But there's nothing preventing somebody
from calling red_channel_client_prepare_out_msg() with a smaller 'vec'
array, which always makes me just a bit nervous. Not sure we need to
fix anything here necessarily, but just thought I'd mention it.
Jonathon
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel