| Message ID | 20180301084438.13594-4-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qio: general non-default GMainContext support | expand |
On Thu, Mar 01, 2018 at 04:44:26PM +0800, Peter Xu wrote: > Firstly, introduce an internal qio_channel_add_watch_full(), which > enhances qio_channel_add_watch() that context can be specified. > > Then add a new API wrapper qio_channel_add_watch_source() to return a > GSource pointer rather than a tag ID. > > Note that the _source() call will keep a reference of GSource so that > callers need to unref them explicitly when finished using the GSource. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/io/channel.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ > io/channel.c | 40 ++++++++++++++++++++++++++++++++++------ > 2 files changed, 78 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On 01/03/2018 09:44, Peter Xu wrote: > + * qio_channel_add_watch_source: > + * @ioc: the channel object > + * @condition: the I/O condition to monitor > + * @func: callback to invoke when the source becomes ready > + * @user_data: opaque data to pass to @func > + * @notify: callback to free @user_data > + * @context: gcontext to bind the source to > + * > + * Similar as qio_channel_add_watch(), but allows to specify context > + * to run the watch source, meanwhile return the GSource object > + * instead of tag ID, with the GSource referenced already. > + * > + * Note: callers is responsible to unref the source when not needed. > + * > + * Returns: the source pointer > + */ > +GSource *qio_channel_add_watch_source(QIOChannel *ioc, > + GIOCondition condition, > + QIOChannelFunc func, > + gpointer user_data, > + GDestroyNotify notify, > + GMainContext *context); > Just a small thing, this is a bit inconsistent with the rest of the GSource API, where the g_source_attach is usually left to the caller when a function returns GSource *. You might therefore name it instead qio_channel_create_watch, for consistency with g_io_{add,create}_watch, and remove the "context" argument. (Not making it perfectly the same API is okay, for example in practice all callers would use g_source_set_callback so it's okay IMO to add the three arguments func/user_data/notify. However, inconsistency on g_source_add is more subtle). Thanks, Paolo
On Thu, Mar 01, 2018 at 06:13:06PM +0100, Paolo Bonzini wrote: > On 01/03/2018 09:44, Peter Xu wrote: > > + * qio_channel_add_watch_source: > > + * @ioc: the channel object > > + * @condition: the I/O condition to monitor > > + * @func: callback to invoke when the source becomes ready > > + * @user_data: opaque data to pass to @func > > + * @notify: callback to free @user_data > > + * @context: gcontext to bind the source to > > + * > > + * Similar as qio_channel_add_watch(), but allows to specify context > > + * to run the watch source, meanwhile return the GSource object > > + * instead of tag ID, with the GSource referenced already. > > + * > > + * Note: callers is responsible to unref the source when not needed. > > + * > > + * Returns: the source pointer > > + */ > > +GSource *qio_channel_add_watch_source(QIOChannel *ioc, > > + GIOCondition condition, > > + QIOChannelFunc func, > > + gpointer user_data, > > + GDestroyNotify notify, > > + GMainContext *context); > > > > Just a small thing, this is a bit inconsistent with the rest of the > GSource API, where the g_source_attach is usually left to the caller > when a function returns GSource *. > > You might therefore name it instead qio_channel_create_watch, for > consistency with g_io_{add,create}_watch, and remove the "context" argument. Looks like there is already a qio_channel_create_watch() (io/channel.c). How about qio_channel_create_watch_attached()? Or... anything better? Thanks,
On 02/03/2018 04:54, Peter Xu wrote: > On Thu, Mar 01, 2018 at 06:13:06PM +0100, Paolo Bonzini wrote: >> On 01/03/2018 09:44, Peter Xu wrote: >>> + * qio_channel_add_watch_source: >>> + * @ioc: the channel object >>> + * @condition: the I/O condition to monitor >>> + * @func: callback to invoke when the source becomes ready >>> + * @user_data: opaque data to pass to @func >>> + * @notify: callback to free @user_data >>> + * @context: gcontext to bind the source to >>> + * >>> + * Similar as qio_channel_add_watch(), but allows to specify context >>> + * to run the watch source, meanwhile return the GSource object >>> + * instead of tag ID, with the GSource referenced already. >>> + * >>> + * Note: callers is responsible to unref the source when not needed. >>> + * >>> + * Returns: the source pointer >>> + */ >>> +GSource *qio_channel_add_watch_source(QIOChannel *ioc, >>> + GIOCondition condition, >>> + QIOChannelFunc func, >>> + gpointer user_data, >>> + GDestroyNotify notify, >>> + GMainContext *context); >>> >> >> Just a small thing, this is a bit inconsistent with the rest of the >> GSource API, where the g_source_attach is usually left to the caller >> when a function returns GSource *. >> >> You might therefore name it instead qio_channel_create_watch, for >> consistency with g_io_{add,create}_watch, and remove the "context" argument. > > Looks like there is already a qio_channel_create_watch() (io/channel.c). > > How about qio_channel_create_watch_attached()? Or... anything better? I would add the g_source_set_callback call (and arguments) to qio_channel_create_watch and leave the g_source_attach to the caller. Paolo
On Thu, Mar 01, 2018 at 06:13:06PM +0100, Paolo Bonzini wrote: > On 01/03/2018 09:44, Peter Xu wrote: > > + * qio_channel_add_watch_source: > > + * @ioc: the channel object > > + * @condition: the I/O condition to monitor > > + * @func: callback to invoke when the source becomes ready > > + * @user_data: opaque data to pass to @func > > + * @notify: callback to free @user_data > > + * @context: gcontext to bind the source to > > + * > > + * Similar as qio_channel_add_watch(), but allows to specify context > > + * to run the watch source, meanwhile return the GSource object > > + * instead of tag ID, with the GSource referenced already. > > + * > > + * Note: callers is responsible to unref the source when not needed. > > + * > > + * Returns: the source pointer > > + */ > > +GSource *qio_channel_add_watch_source(QIOChannel *ioc, > > + GIOCondition condition, > > + QIOChannelFunc func, > > + gpointer user_data, > > + GDestroyNotify notify, > > + GMainContext *context); > > > > Just a small thing, this is a bit inconsistent with the rest of the > GSource API, where the g_source_attach is usually left to the caller > when a function returns GSource *. The APIs which return a GSource in glib typically don't even set the callback function - we already cover that scenario with the qio_channel_create_watch APIs. GLib doesn't typically have APIs which return a GSource after the mix of creating the watch, setting callback & attaching to context. They all just return the watch ID value. So I think this proposal is ok as it is as there's no real precedence. Alternatively we could simply do without this API entirely. It is trivial enough for the code that needs a GSource to get iuse the normal qio_channel_add_watch|watch_full APIs, and then lookup the GSource themselves - only one extra line of code in the callers. Regards, Daniel
On 02/03/2018 16:44, Daniel P. Berrangé wrote: >> Just a small thing, this is a bit inconsistent with the rest of the >> GSource API, where the g_source_attach is usually left to the caller >> when a function returns GSource *. > The APIs which return a GSource in glib typically don't even set the > callback function - we already cover that scenario with the > qio_channel_create_watch APIs. > > GLib doesn't typically have APIs which return a GSource after the > mix of creating the watch, setting callback & attaching to context. True, on the other hand it's annoying for qio_channel_create_watch because of the function pointer case. So I suggested a mix of creating the watch and setting the callback. Having a function that takes a GMainContext and returns a tag is fine, too. Paolo > They all just return the watch ID value. > > So I think this proposal is ok as it is as there's no real precedence. > > Alternatively we could simply do without this API entirely. It is > trivial enough for the code that needs a GSource to get iuse the > normal qio_channel_add_watch|watch_full APIs, and then lookup the > GSource themselves - only one extra line of code in the callers.
diff --git a/include/io/channel.h b/include/io/channel.h index 3995e243a3..e8cdadb0b0 100644 --- a/include/io/channel.h +++ b/include/io/channel.h @@ -648,6 +648,50 @@ guint qio_channel_add_watch(QIOChannel *ioc, gpointer user_data, GDestroyNotify notify); +/** + * qio_channel_add_watch_full: + * @ioc: the channel object + * @condition: the I/O condition to monitor + * @func: callback to invoke when the source becomes ready + * @user_data: opaque data to pass to @func + * @notify: callback to free @user_data + * @context: the context to run the watch source + * + * Similar as qio_channel_add_watch(), but allows to specify context + * to run the watch source. + * + * Returns: the source ID + */ +guint qio_channel_add_watch_full(QIOChannel *ioc, + GIOCondition condition, + QIOChannelFunc func, + gpointer user_data, + GDestroyNotify notify, + GMainContext *context); + +/** + * qio_channel_add_watch_source: + * @ioc: the channel object + * @condition: the I/O condition to monitor + * @func: callback to invoke when the source becomes ready + * @user_data: opaque data to pass to @func + * @notify: callback to free @user_data + * @context: gcontext to bind the source to + * + * Similar as qio_channel_add_watch(), but allows to specify context + * to run the watch source, meanwhile return the GSource object + * instead of tag ID, with the GSource referenced already. + * + * Note: callers is responsible to unref the source when not needed. + * + * Returns: the source pointer + */ +GSource *qio_channel_add_watch_source(QIOChannel *ioc, + GIOCondition condition, + QIOChannelFunc func, + gpointer user_data, + GDestroyNotify notify, + GMainContext *context); /** * qio_channel_attach_aio_context: diff --git a/io/channel.c b/io/channel.c index ec4b86de7c..8dd0684f5d 100644 --- a/io/channel.c +++ b/io/channel.c @@ -299,11 +299,12 @@ void qio_channel_set_aio_fd_handler(QIOChannel *ioc, klass->io_set_aio_fd_handler(ioc, ctx, io_read, io_write, opaque); } -guint qio_channel_add_watch(QIOChannel *ioc, - GIOCondition condition, - QIOChannelFunc func, - gpointer user_data, - GDestroyNotify notify) +guint qio_channel_add_watch_full(QIOChannel *ioc, + GIOCondition condition, + QIOChannelFunc func, + gpointer user_data, + GDestroyNotify notify, + GMainContext *context) { GSource *source; guint id; @@ -312,12 +313,39 @@ guint qio_channel_add_watch(QIOChannel *ioc, g_source_set_callback(source, (GSourceFunc)func, user_data, notify); - id = g_source_attach(source, NULL); + id = g_source_attach(source, context); g_source_unref(source); return id; } +guint qio_channel_add_watch(QIOChannel *ioc, + GIOCondition condition, + QIOChannelFunc func, + gpointer user_data, + GDestroyNotify notify) +{ + return qio_channel_add_watch_full(ioc, condition, func, + user_data, notify, NULL); +} + +GSource *qio_channel_add_watch_source(QIOChannel *ioc, + GIOCondition condition, + QIOChannelFunc func, + gpointer user_data, + GDestroyNotify notify, + GMainContext *context) +{ + GSource *source; + guint id; + + id = qio_channel_add_watch_full(ioc, condition, func, + user_data, notify, context); + source = g_main_context_find_source_by_id(context, id); + g_source_ref(source); + return source; +} + int qio_channel_shutdown(QIOChannel *ioc, QIOChannelShutdown how,
Firstly, introduce an internal qio_channel_add_watch_full(), which enhances qio_channel_add_watch() that context can be specified. Then add a new API wrapper qio_channel_add_watch_source() to return a GSource pointer rather than a tag ID. Note that the _source() call will keep a reference of GSource so that callers need to unref them explicitly when finished using the GSource. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/io/channel.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ io/channel.c | 40 ++++++++++++++++++++++++++++++++++------ 2 files changed, 78 insertions(+), 6 deletions(-)