>
> 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.
See my comment on 00/10. The reason is that mostly is a broken
unused abstraction, that's nothing wrong in the abstract concept
itself.
> > ---
> > 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?
>
I would place a TODO then. I would wait to remove the opaque (some patch
later) before renaming. About name... OutgoingMessageBuffer ?
> > 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)
>
That sound ugly. Why hiding a dependency into a binary dynamic form
is better? See my suggestion on previous patch.
This hunk is not related much to this 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);
Maybe these function should be renamed too. Not much sense to
still call them _on_<something> while we know what we want them
to do. We are removing the abstraction.
Why not red_channel_client_set_blocked ?
> > return;
> > case EINTR:
> > continue;
> > case EPIPE:
> > - handler->cb->on_error(handler->opaque);
> > + /* FIXME: handle disconnection in caller */
What's this FIXME about ?
> > + 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);
red_channel_client_data_sent ?
> > 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);
red_channel_client_msg_sent ?
> > 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
I think this is good for a follow up.
Also note that's always vec == vec_buf and could be an automatic variable
inside red_channel_client_handle_outgoing (although I think it's 16K).
Probably we could have a
static int red_channel_client_prepare_out_msg(RedChannelClient *rcc,
struct iovec *vec, int vec_size,
int pos)
{
return spice_marshaller_fill_iovec(rcc->priv->send_data.marshaller,
vec, vec_size, pos);
}
(and maybe at this point inline it...)
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel