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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> >
>
> Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel