[libvirt] [libvirt-glib 0/3] Fixes to glib/libvirt mainloop integration

While reviewing danpb's win32 patch for the mainloop integration code, I found 2 more issues in the existing code (one leak, one bug). These patches fix this (first patch is just some cleanup). Christophe

It's possible to create a handle to watch for file events which do not watch for any file event. Such a handle can be enabled later with gvir_event_handle_update() by setting some conditions to watch for. When a handle is disabled after it has been created, gvir_event_handle_update() makes sure it removes the corresponding gvir_event_handle::source IO watch if any was set. gvir_event_handle_add() will always create a gvir_event_handle::source IO watch even if the handle is not watching for any events. This commit makes consistent by only creating a watch with g_io_add_watch() when the caller asked to watch for some events. --- libvirt-glib/libvirt-glib-event.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/libvirt-glib/libvirt-glib-event.c b/libvirt-glib/libvirt-glib-event.c index 1e1ffec..67144fa 100644 --- a/libvirt-glib/libvirt-glib-event.c +++ b/libvirt-glib/libvirt-glib-event.c @@ -177,10 +177,12 @@ gvir_event_handle_add(int fd, g_debug("Add handle %p %d %d %d %p\n", data, data->watch, data->fd, events, data->opaque); - data->source = g_io_add_watch(data->channel, - cond, - gvir_event_handle_dispatch, - data); + if (events != 0) { + data->source = g_io_add_watch(data->channel, + cond, + gvir_event_handle_dispatch, + data); + } g_ptr_array_add(handles, data); -- 1.8.5.3

On Tue, Jan 28, 2014 at 02:08:26PM +0100, Christophe Fergeau wrote:
It's possible to create a handle to watch for file events which do not watch for any file event. Such a handle can be enabled later with gvir_event_handle_update() by setting some conditions to watch for.
When a handle is disabled after it has been created, gvir_event_handle_update() makes sure it removes the corresponding gvir_event_handle::source IO watch if any was set. gvir_event_handle_add() will always create a gvir_event_handle::source IO watch even if the handle is not watching for any events.
This commit makes consistent by only creating a watch with g_io_add_watch() when the caller asked to watch for some events. --- libvirt-glib/libvirt-glib-event.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
ACK 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 :|

Trying to remove a disabled timer or handle will cause gvir_{event,timer}_handle_remove() to return an error rather than removing it. --- libvirt-glib/libvirt-glib-event.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libvirt-glib/libvirt-glib-event.c b/libvirt-glib/libvirt-glib-event.c index 67144fa..adcc2fe 100644 --- a/libvirt-glib/libvirt-glib-event.c +++ b/libvirt-glib/libvirt-glib-event.c @@ -292,12 +292,12 @@ gvir_event_handle_remove(int watch) g_debug("Remove handle %p %d %d\n", data, watch, data->fd); - if (!data->source) - goto cleanup; + if (data->source != 0) { + g_source_remove(data->source); + data->source = 0; + data->events = 0; + } - g_source_remove(data->source); - data->source = 0; - data->events = 0; /* since the actual watch deletion is done asynchronously, a handle_update call may * reschedule the watch before it's fully deleted, that's why we need to mark it as * 'removed' to prevent reuse @@ -448,11 +448,11 @@ gvir_event_timeout_remove(int timer) g_debug("Remove timeout %p %d\n", data, timer); - if (!data->source) - goto cleanup; + if (data->source != 0) { + g_source_remove(data->source); + data->source = 0; + } - g_source_remove(data->source); - data->source = 0; /* since the actual timeout deletion is done asynchronously, a timeout_update call may * reschedule the timeout before it's fully deleted, that's why we need to mark it as * 'removed' to prevent reuse -- 1.8.5.3

On Tue, Jan 28, 2014 at 02:08:27PM +0100, Christophe Fergeau wrote:
Trying to remove a disabled timer or handle will cause gvir_{event,timer}_handle_remove() to return an error rather than removing it. --- libvirt-glib/libvirt-glib-event.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
ACK 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 :|

gvir_event_handle_add() creates a GIOChannel in order to watch the fd it was given for changes. gvir_event_handle_remove() is freeing all the resources allocated by gvir_event_handle_add() except for this GIOChannel. This commit adds the needed g_io_channel_unref() call to gvir_event_handle_remove() --- libvirt-glib/libvirt-glib-event.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libvirt-glib/libvirt-glib-event.c b/libvirt-glib/libvirt-glib-event.c index adcc2fe..f8227d6 100644 --- a/libvirt-glib/libvirt-glib-event.c +++ b/libvirt-glib/libvirt-glib-event.c @@ -298,6 +298,10 @@ gvir_event_handle_remove(int watch) data->events = 0; } + g_warn_if_fail(data->channel != NULL); + g_io_channel_unref(data->channel); + data->channel = NULL; + /* since the actual watch deletion is done asynchronously, a handle_update call may * reschedule the watch before it's fully deleted, that's why we need to mark it as * 'removed' to prevent reuse -- 1.8.5.3

On Tue, Jan 28, 2014 at 02:08:28PM +0100, Christophe Fergeau wrote:
gvir_event_handle_add() creates a GIOChannel in order to watch the fd it was given for changes. gvir_event_handle_remove() is freeing all the resources allocated by gvir_event_handle_add() except for this GIOChannel. This commit adds the needed g_io_channel_unref() call to gvir_event_handle_remove() --- libvirt-glib/libvirt-glib-event.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/libvirt-glib/libvirt-glib-event.c b/libvirt-glib/libvirt-glib-event.c index adcc2fe..f8227d6 100644 --- a/libvirt-glib/libvirt-glib-event.c +++ b/libvirt-glib/libvirt-glib-event.c @@ -298,6 +298,10 @@ gvir_event_handle_remove(int watch) data->events = 0; }
+ g_warn_if_fail(data->channel != NULL); + g_io_channel_unref(data->channel); + data->channel = NULL; + /* since the actual watch deletion is done asynchronously, a handle_update call may * reschedule the watch before it's fully deleted, that's why we need to mark it as * 'removed' to prevent reuse
ACK 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 :|
participants (2)
-
Christophe Fergeau
-
Daniel P. Berrange