[Spice-devel] [spice-server 05/10] More removal of IncomingHandlerInterface vfuncs

[Christophe Fergeau]

On Wed, Feb 08, 2017 at 04:43:09PM -0600, Jonathon Jongsma wrote:
> On Tue, 2017-02-07 at 11:59 +0100, Christophe Fergeau wrote:
> > This commit removes IncomingHandlerInterface::on_error and
> > IncomingHandlerInterface::on_input. As with previous commits, these
> > vfuncs are always calling the method, and RedChannel::init sets them
> > to
> > point to RedChannelClient methods, which RedChannelClient is then
> > going
> > to call indirectly through the IncomingHandlerInterface instance.
> > 
> > This commit changes this to direct calls to the corresponding
> > methods.
> > ---
> >  server/red-channel-client.c | 22 ++++++++++++++--------
> >  server/red-channel-client.h |  2 --
> >  server/red-channel.c        | 10 ----------
> >  server/red-channel.h        |  2 --
> >  4 files changed, 14 insertions(+), 22 deletions(-)
> > 
> > diff --git a/server/red-channel-client.c b/server/red-channel-
> > client.c
> > index a617582..0c43c46 100644
> > --- a/server/red-channel-client.c
> > +++ b/server/red-channel-client.c
> > @@ -384,7 +384,7 @@ static void red_channel_client_on_output(void
> > *opaque, int n)
> >      red_channel_on_output(channel, n);
> >  }
> >  
> > -void red_channel_client_on_input(void *opaque, int n)
> > +static void red_channel_client_on_input(void *opaque, int n)
> >  {
> >      RedChannelClient *rcc = opaque;
> >  
> > @@ -1139,10 +1139,12 @@ static void
> > red_peer_handle_incoming(RedsStream *stream, IncomingHandler *handle
> >                                            handler->header.data +
> > handler->header_pos,
> >                                            handler-
> > >header.header_size - handler->header_pos);
> >              if (bytes_read == -1) {
> > -                handler->cb->on_error(handler->opaque);
> > +                /* FIXME: handle disconnection in caller */
> > +                red_channel_client_disconnect(handler->opaque);
> 
> I'm a little bit confused by these FIXME comments. The fact that you've
> added them as part of this commit implies that they're somehow related
> to these changes. But you didn't really change any behavior with this
> patch. And what do they mean exactly? 

See
https://lists.freedesktop.org/archives/spice-devel/2017-February/035612.html
for explanations

« 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 :) »
I guess I'll drop these and keep that in the back of my mind ;)

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170210/6d666280/attachment.sig>