[virt-tools-list] [virt-viewer][PATCH 4/6] events: protect "handles" and "timeouts" agins concurrent access
Victor Toso
victortoso at redhat.com
Fri Jul 17 16:43:25 UTC 2015
Typo in the short commit message: against
On Fri, Jul 17, 2015 at 04:01:21PM +0200, Fabiano Fidêncio wrote:
> Timeout and Handle operations are done from an idle callback. However,
> we cannot assume that all libvirt event calls (the callbacks passed to
> virEventRegisterImpl) will be done from the mainloop thread. It's thus
> possible that a libvirt event call will run a thread while one of the
> callbacks runs.
> Given that "handles" and "timeouts" arrays are shared among all threads,
> we need to make sure we hold the "eventlock" mutex before modifying
> them.
>
> Based on
> http://libvirt.org/git/?p=libvirt-glib.git;a=commit;h=924178f6b35735458b37d30303fe7bc753dde0b1
>
> Related to: rhbz#1243228
> ---
> src/virt-viewer-events.c | 109 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 82 insertions(+), 27 deletions(-)
>
> diff --git a/src/virt-viewer-events.c b/src/virt-viewer-events.c
> index b92506c..ef2c317 100644
> --- a/src/virt-viewer-events.c
> +++ b/src/virt-viewer-events.c
> @@ -33,6 +33,9 @@
> #include <libvirt/libvirt.h>
>
> #include "virt-viewer-events.h"
> +#include "virt-glib-compat.h"
> +
> +GMutex *eventlock = NULL;
>
> struct virt_viewer_events_handle
> {
> @@ -84,6 +87,9 @@ int virt_viewer_events_add_handle(int fd,
> {
> struct virt_viewer_events_handle *data;
> GIOCondition cond = 0;
> + int ret;
> +
> + g_mutex_lock(eventlock);
>
> data = g_new0(struct virt_viewer_events_handle, 1);
>
> @@ -114,7 +120,11 @@ int virt_viewer_events_add_handle(int fd,
>
> g_ptr_array_add(handles, data);
>
> - return data->watch;
> + ret = data->watch;
> +
> + g_mutex_unlock(eventlock);
> +
> + return ret;
> }
>
> static struct virt_viewer_events_handle *
> @@ -144,19 +154,22 @@ static void
> virt_viewer_events_update_handle(int watch,
> int events)
> {
> - struct virt_viewer_events_handle *data = virt_viewer_events_find_handle(watch);
> + struct virt_viewer_events_handle *data;
> +
> + g_mutex_lock(eventlock);
>
> - if (!data) {
> + data = virt_viewer_events_find_handle(watch);
> + if (data == NULL) {
> g_debug("Update for missing handle watch %d", watch);
> - return;
> + goto cleanup;
> }
>
> - if (events) {
> + if (events != 0) {
> GIOCondition cond = 0;
> if (events == data->events)
> - return;
> + goto cleanup;
>
> - if (data->source)
> + if (data->source != 0)
> g_source_remove(data->source);
>
> cond |= G_IO_HUP;
> @@ -170,13 +183,16 @@ virt_viewer_events_update_handle(int watch,
> data);
> data->events = events;
> } else {
> - if (!data->source)
> - return;
> + if (data->source == 0)
> + goto cleanup;
>
> g_source_remove(data->source);
> data->source = 0;
> data->events = 0;
> }
> +
> +cleanup:
> + g_mutex_unlock(eventlock);
> }
>
>
> @@ -191,7 +207,10 @@ virt_viewer_events_cleanup_handle(gpointer user_data)
> if (data->ff)
> (data->ff)(data->opaque);
>
> + g_mutex_lock(eventlock);
> g_ptr_array_remove_fast(handles, data);
> + g_mutex_unlock(eventlock);
> +
> return FALSE;
> }
>
> @@ -199,16 +218,20 @@ virt_viewer_events_cleanup_handle(gpointer user_data)
> static int
> virt_viewer_events_remove_handle(int watch)
> {
> - struct virt_viewer_events_handle *data = virt_viewer_events_find_handle(watch);
> + struct virt_viewer_events_handle *data;
> + int ret = -1;
>
> - if (!data) {
> + g_mutex_lock(eventlock);
> +
> + data = virt_viewer_events_find_handle(watch);
> + if (data == NULL) {
> g_debug("Remove of missing watch %d", watch);
> - return -1;
> + goto cleanup;
> }
>
> g_debug("Remove handle %d %d", watch, data->fd);
>
> - if (data->source) {
> + if (data->source != 0) {
> g_source_remove(data->source);
> data->source = 0;
> data->events = 0;
> @@ -220,7 +243,12 @@ virt_viewer_events_remove_handle(int watch)
> */
> data->removed = TRUE;
> g_idle_add(virt_viewer_events_cleanup_handle, data);
> - return 0;
> + ret = 0;
> +
> +cleanup:
> + g_mutex_unlock(eventlock);
> +
> + return ret;
> }
>
> struct virt_viewer_events_timeout
> @@ -255,6 +283,9 @@ virt_viewer_events_add_timeout(int interval,
> virFreeCallback ff)
> {
> struct virt_viewer_events_timeout *data;
> + int ret;
> +
> + g_mutex_lock(eventlock);
>
> data = g_new0(struct virt_viewer_events_timeout, 1);
>
> @@ -272,7 +303,11 @@ virt_viewer_events_add_timeout(int interval,
>
> g_debug("Add timeout %p %d %p %p %d", data, interval, cb, opaque, data->timer);
>
> - return data->timer;
> + ret = data->timer;
> +
> + g_mutex_unlock(eventlock);
> +
> + return ret;
> }
>
>
> @@ -304,30 +339,36 @@ static void
> virt_viewer_events_update_timeout(int timer,
> int interval)
> {
> - struct virt_viewer_events_timeout *data = virt_viewer_events_find_timeout(timer);
> + struct virt_viewer_events_timeout *data;
> +
> + g_mutex_lock(eventlock);
>
> - if (!data) {
> + data = virt_viewer_events_find_timeout(timer);
> + if (data == NULL) {
> g_debug("Update of missing timer %d", timer);
> - return;
> + goto cleanup;
> }
>
> g_debug("Update timeout %p %d %d", data, timer, interval);
>
> if (interval >= 0) {
> - if (data->source)
> - return;
> + if (data->source != 0)
> + goto cleanup;
>
> data->interval = interval;
> data->source = g_timeout_add(data->interval,
> virt_viewer_events_dispatch_timeout,
> data);
> } else {
> - if (!data->source)
> - return;
> + if (data->source == 0)
> + goto cleanup;
>
> g_source_remove(data->source);
> data->source = 0;
> }
> +
> +cleanup:
> + g_mutex_unlock(eventlock);
> }
>
>
> @@ -342,7 +383,10 @@ virt_viewer_events_cleanup_timeout(gpointer user_data)
> if (data->ff)
> (data->ff)(data->opaque);
>
> + g_mutex_lock(eventlock);
> g_ptr_array_remove_fast(timeouts, data);
> + g_mutex_unlock(eventlock);
> +
> return FALSE;
> }
>
> @@ -350,16 +394,21 @@ virt_viewer_events_cleanup_timeout(gpointer user_data)
> static int
> virt_viewer_events_remove_timeout(int timer)
> {
> - struct virt_viewer_events_timeout *data = virt_viewer_events_find_timeout(timer);
> + struct virt_viewer_events_timeout *data;
> + int ret = -1;
> +
> + g_mutex_lock(eventlock);
>
> - if (!data) {
> + data = virt_viewer_events_find_timeout(timer);
> +
> + if (data == NULL) {
> g_debug("Remove of missing timer %d", timer);
> - return -1;
> + goto cleanup;
> }
>
> g_debug("Remove timeout %p %d", data, timer);
>
> - if (data->source) {
> + if (data->source != 0) {
> g_source_remove(data->source);
> data->source = 0;
> }
> @@ -370,11 +419,17 @@ virt_viewer_events_remove_timeout(int timer)
> */
> data->removed = TRUE;
> g_idle_add(virt_viewer_events_cleanup_timeout, data);
> - return 0;
> + ret = 0;
> +
> +cleanup:
> + g_mutex_unlock(eventlock);
> +
> + return ret;
> }
>
> static gpointer event_register_once(gpointer data G_GNUC_UNUSED)
> {
> + eventlock = g_mutex_new();
> timeouts = g_ptr_array_new_with_free_func(g_free);
> handles = g_ptr_array_new_with_free_func(g_free);
> virEventRegisterImpl(virt_viewer_events_add_handle,
> --
> 2.4.4
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
More information about the virt-tools-list
mailing list