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

[Frediano Ziglio]

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

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