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

[Jonathon Jongsma]

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.
----

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?

> > > + * 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