Re: [Qemu-devel] [PATCH v2 03/15] qio: introduce qio_channel_add_watch_{full|source}

["Daniel P. Berrangé" <berrange@redhat.com>]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|