[Spice-devel] [PATCH 02/10] Move RedChannelClient to separate file

[Jonathon Jongsma]

On Thu, 2016-09-01 at 17:45 +0200, Christophe Fergeau wrote:
> On Wed, Aug 31, 2016 at 11:54:38AM -0500, Jonathon Jongsma wrote:
> > 
> > Reduce direct access to RedChannelClient, and get ready to convert
> > to
> > GObject.
> > ---

[snip]

> > diff --git a/server/red-channel-client.c b/server/red-channel-
> > client.c
> > new file mode 100644
> > index 0000000..253e30e
> > --- /dev/null
> > +++ b/server/red-channel-client.c
> > @@ -0,0 +1,1626 @@
> > +
> > +static void red_channel_handle_migrate_flush_mark(RedChannelClient
> > *rcc)
> > +{
> > +    RedChannel *channel = red_channel_client_get_channel(rcc);
> > +    if (channel->channel_cbs.handle_migrate_flush_mark) {
> > +        channel->channel_cbs.handle_migrate_flush_mark(rcc);
> > +    }
> > +}
> > +
> > +// TODO: the whole migration is broken with multiple clients. What
> > do we want to do?
> > +// basically just
> > +//  1) source send mark to all
> > +//  2) source gets at various times the data (waits for all)
> > +//  3) source migrates to target
> > +//  4) target sends data to all
> > +// So need to make all the handlers work with per channel/client
> > data (what data exactly?)
> > +static void red_channel_handle_migrate_data(RedChannelClient *rcc,
> > uint32_t size, void *message)
> 
> 
> Just to be sure, these 2 functions were intended to be moved? They
> are
> not in the red_channel_client_ namespace but in the red_channel_ one.
> 


Hmm, yeah. I should have renamed these to
red_channel_client_handle_migrate...


[snip]


> > diff --git a/server/red-channel.c b/server/red-channel.c
> > index bf290b1..03338aa 100644
> > --- a/server/red-channel.c
> > +++ b/server/red-channel.c
> > @@ -2179,11 +707,7 @@ void
> > red_client_semi_seamless_migrate_complete(RedClient *client)
> >      link = client->channels;
> >      while (link) {
> >          next = link->next;
> > -        RedChannelClient *rcc = link->data;
> > -
> > -        if (rcc->latency_monitor.timer) {
> > -            red_channel_client_start_ping_timer(rcc,
> > PING_TEST_IDLE_NET_TIMEOUT_MS);
> > -        }
> > +        red_channel_client_semi_seamless_migration_complete(link-
> > >data);
> 
> Non-obvious name given the code it replaces, but I guess you
> double-checked that's a good name?
> 


I see your point. Basically I needed to avoid modifying the
RedChannelClient object from within the RedClient implementation.
Before, there was a single function that accessed internal data of both
the RedClient and RedChannelClient objects:
red_client_semi_seamless_migrate_complete(). So I simply split the
function into two parts, each responsible for accessing the internals
of its own object:
 - red_client_semi_seamless_migrate_complete() - handles the RedClient
part
 - red_channel_client_semi_seamless_migration_complete() - handles the
RedChannelClient part

That was my justification for the name anyway. I'm open to alternative
suggestions. I believe I considered something like
_on_semi_seamless_migration_complete(), but didn't use that name. The
_on_ terminology might make it more obvious that this function is sort
of an 'event handler' to be called when semi-seamless migration is
complete?



> > -// TODO: again - what is the context exactly? this happens in
> > channel disconnect. but our
> > -// current red_channel_shutdown also closes the socket - is there
> > a socket to close?
> > -// are we reading from an fd here? arghh
> > -void red_channel_client_pipe_clear(RedChannelClient *rcc);
> 
> Note that this comment is attached to
> red_channel_client_pipe_clear().
> In the new header, _pipe_clear() is static and the header now has
> 
> > 
> > +// TODO: again - what is the context exactly? this happens in
> > channel disconnect. but our
> > +// current red_channel_shutdown also closes the socket - is there
> > a socket to close?
> > +// are we reading from an fd here? arghh
> > +void red_channel_client_receive(RedChannelClient *rcc);

Oops, I'll move that comment down to the static _pipe_clear() function.

Jonathon