[Spice-devel] [PATCH 00/26] final part of red_channel refactor

[Alon Levy]

On Tue, Feb 15, 2011 at 02:28:48AM +0100, Marc-André Lureau wrote:
> Hi Alon,
> 
> On Fri, Feb 11, 2011 at 6:48 PM, Alon Levy <alevy at redhat.com> wrote:
> > Starts with using RedChannel in red_worker.c, then tries to use the marshaller
> > all the way [1]. The later parts are mainly for adding multiple clients later.
> >
> > This patchset is basically right before the patch that adds the
> > RedChannelClient struct (although it is only really useful quite a few patches
> > later). The tree with the complete multi client support is here (only
> > reference, still quite dirty with debug stuff and a few wrong turns):
> >
> >  git.freedesktop.org/~alon/spice server_multi_client.v2.refactor.wip
> >
> > [1]  I'm just going over these patches that I haven't touched in a few months,
> > sorry for the lame description.
> 
> There are a few things that annoys me in this patch series (see
> individual patch comments):
> 
> - apparently, patch 22-26 are missing form the ML. This is not a
> problem since you can already take some of the review comments and
> resend the patch series later.
> 
> - some patches are really non-trivial for me, please give me a bit
> more context :) - see my comments
> 
> - the almost systematic refactoring of OO C methods of the form
> my_foo_call(foo) to something with less encapsulation, like:
> my_foo_call(foo, arg1, arg2), where args are in fact foo members, and
> can be reached from foo directly. Perhaps the objects are lacking some
> accessors to make life easier?
> 
> - some renaming lack consistency
> 

I'll address the trivial things. The less trivial I've answered to individual
patch review emails. Generally the whole de-encapsulation thing is explained by:
 * small changes (in the case of peer in set_socket_options (or whatever it's called))
 * marshaller: it is only valid during send call (when we ask the channel to
 really fill it with bytes to be sent), it should not have an accessor. Or
 if it does it needs to say "please call and use me only during send".

Alon

> regards
> 
> >
> > Alon Levy (26):
> >  server/red_channel: protect red_channel_destroy from NULL peer
> >  server/red_channel (all): change send_pipe_item to add marshaller to
> >    sig
> >  server/red_channel (all): handle MIGRATE_DATA and MIGRATE_FLUSH_DATA
> >  ring: add RING_FOREACH{,_SAFE,_REVERSED}
> >  server: add missing include to ring.h
> >  server/red_channel (all): add peer to config_socket sig
> >  server/tunnel: pass SpiceMarshaller reference from send
> >  server/red_channel (+): remove red_channel_add_buf
> >  server/red_channel: add red_channel_set_shut and use in
> >    inputs_channel
> >  server/red_channel: add red_channel_get_first_socket
> >  server/red_channel (all): pass header in channel_send_pipe_item_proc
> >  server/red_worker: cursor channel: replace _send_ with _marshall_
> >  server/red_channel: add red_channel_all_blocked
> >  server/red_channel: add red_channel_send_message_pending
> >  server/red_worker: replace _send_ functions by _marshall_
> >  server/red_worker: complete removal of send_data.marshaller use
> >  server/red_channel: add red_channel_{,no_}item_being_sent
> >  server/red_worker: remove RedChannel argument from add_buf_from_info
> >  server/red_channel: move free of SET_ACK pipe item to release
> >  server/red_channel: reset send_data.item to NULL after release
> >  server/red_channel: add red_channel_disconnect, use in red_worker
> >  server/red_worker: use red_channel_is_connected
> >  server/red_channel: no opaque in red_channel_peer_on_*_error
> >  server/red_channel: split Incoming/Outgoing to callback and state
> >  server/red_channel: move out_bytes_counter from Outgoing to
> >    RedChannel
> >  server/red_worker: use red_channel_pipe_item_init
> >
> >  common/ring.h              |   21 ++
> >  server/inputs_channel.c    |   21 +-
> >  server/main_channel.c      |  151 ++++++-----
> >  server/red_channel.c       |  192 ++++++++++----
> >  server/red_channel.h       |   98 ++++++--
> >  server/red_client_cache.h  |    2 +-
> >  server/red_tunnel_worker.c |  209 ++++++++--------
> >  server/red_worker.c        |  607 ++++++++++++++++++++++---------------------
> >  server/smartcard.c         |   37 ++--
> >  9 files changed, 764 insertions(+), 574 deletions(-)
> >
> > --
> > 1.7.4
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> 
> 
> 
> -- 
> Marc-André Lureau