[Spice-devel] [PATCH 4/4] add some comments to cursor-channel.h header

[Frediano Ziglio]

> 
> On Thu, 2016-05-26 at 06:59 -0400, Frediano Ziglio wrote:
> > > 
> > > On Fri, 2016-05-13 at 10:16 +0100, Frediano Ziglio wrote:
> > > > Explain usage of the class.
> > > > 
> > > > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > > > ---
> > > >  server/cursor-channel.h | 30 +++++++++++++++++++++++++++++-
> > > >  1 file changed, 29 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/server/cursor-channel.h b/server/cursor-channel.h
> > > > index 6c89bc3..7e801a4 100644
> > > > --- a/server/cursor-channel.h
> > > > +++ b/server/cursor-channel.h
> > > > @@ -23,20 +23,48 @@
> > > >  #include "red-worker.h"
> > > >  #include "red-parse-qxl.h"
> > > >  
> > > > +/**
> > > > + * This type it's a RedChannel class which implement cursor (mouse)
> > > 
> > > it's -> is
> > > implement -> implements
> > > 
> > > > + * movements.
> > > > + * A pointer to CursorChannel can be converted to a RedChannel.
> > > > + */
> > > >  typedef struct CursorChannel CursorChannel;
> > > >  
> > > > +/**
> > > > + * Create CursorChannel.
> > > > + * CursorChannel is intended to be run in a separate thread so
> > > > + * so users of this class should attempt to serialize the class
> > > > + * execution and setup client callbacks after creating the class
> > > > + * using cursor_channel_client_migrate and cursor_channel_connect
> > > > + * as helpers.
> > > > + */
> > > 
> > > I'm afraid I don't fully understand what this comment is trying to
> > > communicate.
> > > 
> > 
> > It's hard to explain in my natural language more in English.
> > 
> > A RedChannel is mainly executed in a single thread, with the exception
> > of the client callbacks, as described in red-channel.h:
> > 
> > /*
> >  * callbacks that are triggered from client events.
> >  * They should be called from the thread that handles the RedClient
> >  */
> > typedef struct {
> >     channel_client_connect_proc connect;
> >     channel_client_disconnect_proc disconnect;
> >     channel_client_migrate_proc migrate;
> > } ClientCbs;
> > 
> > mostly channels execute in the main thread (like MainChannel,
> > InputsChannels)
> > which is the same RedClient is executed however CursorChannel and
> > DisplayChannel are executed in the worker thread (RedWorker).
> > Now, if a RedChannel code has to be executed in a single thread and if
> > RedClient is calling these callbacks from its thread (which in case of
> > CursorChannel and DisplayChannel is different) how are handled these
> > functions?
> > Basically for CursorChannel and DisplayChannel these callbacks are
> > provided by RedQXL (see red_qxl_init in red-qxl.c). These callbacks
> > dispatch the request using the dispatcher to RedWorker (see for
> > instance red_qxl_cursor_migrate) where an handler (like
> > handle_dev_cursor_migrate) calls the correct CursorChannel/DisplayChannel
> > callback (like cursor_channel_client_migrate).
> > Basically RedQXL work as a wrapper to these function serializing their
> > execution.
> > Citing design pattern this would be better to be designed as a Decorator
> > pattern adding serialization to CursorChannel/DisplayChannel instead
> > of exposing the functions as normal functions and having to replace
> > the callbacks. This would require having a proper interface for
> > client callbacks and as a good consequence RedClient would be forced
> > to use the safe (threading speaking) interface instead of possibly
> > being able to call any RedChannel functions.
> > Also this would avoid to expose these callback in the headers.
> > 
> > Hope that this description helps to get some better comment describing
> > the reason of these functions. Any suggestion for the comment are
> > welcome.
> 
> OK. I had to read this over quite a few times, but I think I understand the
> point now. And it means that my comment on the previous patch is probably a
> bit
> off-base.  How about something like this (hopefully I understood things
> properly
> and the following is actually true)?
> 
> ----
> Since CursorChannel is intended to be run in a separate thread, it does not
> register its own client callbacks since they would be called from a different
> thread. Therefore users of this class are responsible for registering their
> own
> client callbacks for CursorChannel. These 'wrapper' client callbacks must
> forward execution on to the CursorChannel thread.
> cursor_channel_client_migrate() and cursor_channel_connect() are provided as
> helper functions and should only be called from the CursorChannel thread.
> ----
> 

Yes, that's fine. Basically the wrapper function should call these helpers
from the proper thread (or serialize in some way).

> Not sure whether that's more clear to others, but it seems a bit more clear
> to
> me (assuming I've understood things properly).
> 
> > 
> > > >  CursorChannel*       cursor_channel_new         (RedWorker *worker);
> > > > -void                 cursor_channel_disconnect  (CursorChannel
> > > > *cursor_channel);
> > > > +
> > > > +/**
> > > > + * Cause the channel to disconnect all clients
> > > > + */
> > > > +void                 cursor_channel_disconnect  (CursorChannel
> > > > *cursor);
> > > >  void                 cursor_channel_reset       (CursorChannel
> > > >  *cursor);
> > > >  void                 cursor_channel_init        (CursorChannel
> > > >  *cursor);
> > > >  void                 cursor_channel_process_cmd (CursorChannel
> > > >  *cursor,
> > > > RedCursorCmd *cursor_cmd);
> > > >  void                 cursor_channel_set_mouse_mode(CursorChannel
> > > >  *cursor,
> > > > uint32_t mode);
> > > > +
> > > > +/**
> > > > + * Connect a new client to CursorChannel.
> > > > + * This is the equivalent of ReChannel client connect callback.
> 
> Add short comment about how it can only be called from cursor channel thread?
> 

I think this is up to the caller as it's caller responsibility.

> > > > + * See comment on cursor_channel_new.
> > > > + */
> > > >  void                 cursor_channel_connect     (CursorChannel
> > > >  *cursor,
> > > > RedClient *client,
> > > >                                                   RedsStream *stream,
> > > >                                                   int migrate,
> > > >                                                   uint32_t
> > > >                                                   *common_caps,
> > > >                                                   int
> > > > num_common_caps,
> > > >                                                   uint32_t *caps, int
> > > > num_caps);
> > > >  
> > > > +/**
> > > > + * Migrate a client channel from a CursorChannel.
> > > > + * This is the equivalent of ReChannel client migrate callback.
> 
> same?
> 
> > > > + * See comment on cursor_channel_new.
> > > > + */
> > > >  void                 cursor_channel_client_migrate(RedChannelClient
> > > >  *client);
> > > >  
> > > >  #endif /* CURSOR_CHANNEL_H_ */
> > > 
> > > 
> > > Otherwise looks fine.
> > > 
> > > Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> > > 
> > 

Frediano