[libvirt] [PATCH libvirt-glib 2/5] Add support for creating watches on streams

[Daniel P. Berrange]

On Mon, Nov 28, 2011 at 06:52:43PM +0100, Christophe Fergeau wrote:
> On Mon, Nov 28, 2011 at 01:13:45PM +0000, Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange at redhat.com>
> > 
> > The GIO GInputStream/GOutputStream async model for I/O does not
> > work for working with non-blocking bi-directional streams. To
> > allow that to be done more effectively, add an API to allow
> > main loop watches to be registered against streams.
> > 
> > Since the libvirt level virStreamEventAddCallback API only allows
> > a single callback to be registered to a stream at any time, the
> > GVirStream object needs to be multiplexing of multiple watches into
> > a single libvirt level callback.
> > 
> > Watches can be removed in the normal way with g_source_remove
> > 
> > * libvirt-gobject/libvirt-gobject-stream.c,
> >   libvirt-gobject/libvirt-gobject-stream.h,
> >   libvirt-gobject/libvirt-gobject.sym: Add gvir_stream_add_watch
> > ---
> >  libvirt-gobject/libvirt-gobject-stream.c |  180 ++++++++++++++++++++++++++++++
> >  libvirt-gobject/libvirt-gobject-stream.h |   17 +++
> >  libvirt-gobject/libvirt-gobject.sym      |    1 +
> >  3 files changed, 198 insertions(+), 0 deletions(-)

> > @@ -199,6 +212,14 @@ static void gvir_stream_finalize(GObject *object)
> >          virStreamFree(priv->handle);
> >      }
> >  
> > +    tmp = priv->sources;
> > +    while (tmp) {
> > +        GVirStreamSource *source = tmp->data;
> > +        g_source_remove(g_source_get_id((GSource*)source));
> 
> I think g_source_destroy can be used here

Yes, I believe so.


> > +static void gvir_stream_source_finalize(GSource *source)
> > +{
> > +    GVirStreamSource *gsource = (GVirStreamSource*)source;
> > +    GVirStreamPrivate *priv = gsource->stream->priv;
> > +    GList *tmp, *prev = NULL;
> > +
> > +    tmp = priv->sources;
> > +    while (tmp) {
> > +        if (tmp->data == source) {
> > +            if (prev) {
> > +                prev->next = tmp->next;
> > +            } else {
> > +                priv->sources = tmp->next;
> > +            }
> > +            tmp->next = NULL;
> > +            g_list_free(tmp);
> > +            break;
> > +        }
> > +
> > +        prev = tmp;
> > +        tmp = tmp->next;
> > +    }
> 
> isn't it doing the same as g_list_remove?

Yes, indeed it is. I will simplify that.

> 
> > +
> > +    gvir_stream_update_events(gsource->stream);
> > +}
> > +
> > +GSourceFuncs gvir_stream_source_funcs = {
> > +    .prepare = gvir_stream_source_prepare,
> > +    .check = gvir_stream_source_check,
> > +    .dispatch = gvir_stream_source_dispatch,
> > +    .finalize = gvir_stream_source_finalize,
> > +};
> > +
> > +gint gvir_stream_add_watch(GVirStream *stream,
> > +                           GVirStreamIOCondition cond,
> > +                           GVirStreamIOFunc func,
> > +                           gpointer opaque,
> > +                           GDestroyNotify notify)
> 
> Dunno if it's worth having both gvir_stream_add_watch and
> gvir_stream_add_watch_full to be consistent with most glib source functions
> (g_timeout_add, g_idle_add, g_io_add_watch, ...). The notify argument would
> only be in the _full variant.

Consistency is always nice, so I'll add a _full variant.

> > +{
> > +    GVirStreamPrivate *priv = stream->priv;
> > +    gint id;
> > +    GVirStreamSource *source = (GVirStreamSource*)g_source_new(&gvir_stream_source_funcs,
> > +                                                               sizeof(GVirStreamSource));
> > +
> > +    source->stream = stream;
> > +    source->cond = cond;
> > +
> > +    priv->sources = g_list_append(priv->sources, source);
> > +
> > +    gvir_stream_update_events(source->stream);
> > +
> > +    g_source_set_callback((GSource*)source, (GSourceFunc)func, opaque, notify);
> > +    g_source_attach((GSource*)source, g_main_context_default());
> > +
> > +    id = g_source_get_id((GSource*)source);
> 
> g_source_attach returns this id which is of type guint.

Ahh, didn't notice that.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|