On Fri, Feb 10, 2017 at 04:47:15AM -0500, Frediano Ziglio wrote:
> >
> > 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.
I'll replace "Since the content of OutgoingHandlerInterface is never modified, we can
directly call the relevant methods and make them static."
with
"The OutgoingHandlerInterface abstraction is unused, ie the codebase
only has a single implementation for it, so we can directly call the
relevant methods and make them static instead", does that read better to
you?
>
> > > ---
> > > 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 ?
I picked that name and queued that at the end of this series (together
with removing red-channel-client-private.h)
> > > -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?
red_channel_on_output() is currently used for statistics gathering purpose.
I do not see much value in hardcoding that RedChannel is gathering
statistics for RedChannelClient in the code. Emitting a signal instead
remove this coupling, and makes things more flexible with respect to
what exact class is going to implement the statistics gathering (aka
observer pattern if I'm not mistaken ?)
>
> > > 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 ?
Ah, did not really give any thoughts about that, I can look into
renaming them indeed, thanks for the name suggestions!
>
> > > return;
> > > case EINTR:
> > > continue;
> > > case EPIPE:
> > > - handler->cb->on_error(handler->opaque);
> > > + /* FIXME: handle disconnection in caller */
>
> What's this FIXME about ?
Ah, this got Jonathon confused later on too, my feeling is that things
would look better as:
int status = red_peer_handle_outgoing(..)
switch (status) {
case 0:
red_channel_client_on_out_msg_done(red_channel_client);
break;
case EPIPE:
default:
red_channel_disconnect(red_channel_client);
break;
}
and limit as much as possible calls to red_channel_client_xxx from
red_peer_handle_outgoing(). Did not explore this yet, so cannot tell
if it makes sense or not :)
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel