>
> 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?
>
Sure. Not only unused, even badly defined.
>
> >
> > > > ---
> > > > 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)
>
Yes, that header is just included by red-channel-client.c so not
hard to remove, just inline it :)
> > > > -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 ?)
>
So are you saying you prefer an heavy unsafe signal instead of a
function call just for statistics that's disabled by default?
Actually is weird considering lot of these patches are removing
callbacks and indirection.
I don't see much problems if RedChannelClient do some statistics,
I was thinking something like
static void red_channel_client_on_output(RedChannelClient *rcc, int n)
{
if (rcc->priv->connectivity_monitor.timer) {
rcc->priv->connectivity_monitor.out_bytes += n;
}
stat_inc_counter(whatever_works, rcc->priv->out_bytes_counter, n);
}
<OT>
Why we need to update a byte count for monitoring?
Is there no such statistics in RedsStream ?
</OT>
> >
> > > > 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
>
Now that the function does a lot direct calls I don't see much sense.
Maybe you can use a kernel style label + error handling code?
In this function there are just 2 calls to red_channel_disconnect.
I was confused by the FIXME which is quite strong (I usually use TODO
if I can improve something, FIXME for possible bugs to fix ASAP) and
that adding make think that the problem was not present before the
patch (that is a regression introduced).
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel