On Fri, Jul 17, 2015 at 6:15 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote: > Hey, > > On Fri, Jul 17, 2015 at 04:01:19PM +0200, Fabiano Fidêncio wrote: >> Based on >> http://libvirt.org/git/?p=libvirt-glib.git;a=commit;h=cff5f1c46f4b9661e112b85159fb58ae473a9a89 > > I'd tend to c&p the initial log too Not sure if the initial log would make sense here. But I can c&p it, no problem. > >> >> Related to: rhbz#1243228 >> --- >> src/virt-viewer-events.c | 97 +++++++++++++++++++++++++++++++----------------- >> 1 file changed, 63 insertions(+), 34 deletions(-) >> >> diff --git a/src/virt-viewer-events.c b/src/virt-viewer-events.c >> index 6154353..b92506c 100644 >> --- a/src/virt-viewer-events.c >> +++ b/src/virt-viewer-events.c >> @@ -39,7 +39,7 @@ struct virt_viewer_events_handle >> int watch; >> int fd; >> int events; >> - int enabled; >> + int removed; > > This (and other parts of this commit) are > https://libvirt.org/git/?p=libvirt-glib.git;a=commitdiff;h=3e73e0cee977fb20dd29db3ccfe85b00cc386c43 > they should go in a separate commit As it was not exactly cherry-picked from the libvirt-glib commits, I don't see a good reason for not having all the fixes squashed when they make sense to be squashed. > >> GIOChannel *channel; >> guint source; >> virEventHandleCallback cb; >> @@ -48,8 +48,7 @@ struct virt_viewer_events_handle >> }; >> >> static int nextwatch = 1; >> -static unsigned int nhandles = 0; >> -static struct virt_viewer_events_handle **handles = NULL; >> +GPtrArray *handles; > > static GPtrArray > >> >> static gboolean >> virt_viewer_events_dispatch_handle(GIOChannel *source G_GNUC_UNUSED, >> @@ -86,9 +85,7 @@ int virt_viewer_events_add_handle(int fd, >> struct virt_viewer_events_handle *data; >> GIOCondition cond = 0; >> >> - handles = g_realloc(handles, sizeof(*handles)*(nhandles+1)); >> - data = g_malloc(sizeof(*data)); >> - memset(data, 0, sizeof(*data)); >> + data = g_new0(struct virt_viewer_events_handle, 1); >> >> if (events & VIR_EVENT_HANDLE_READABLE) >> cond |= G_IO_IN; >> @@ -115,7 +112,7 @@ int virt_viewer_events_add_handle(int fd, >> virt_viewer_events_dispatch_handle, >> data); >> >> - handles[nhandles++] = data; >> + g_ptr_array_add(handles, data); >> >> return data->watch; >> } >> @@ -123,10 +120,22 @@ int virt_viewer_events_add_handle(int fd, >> static struct virt_viewer_events_handle * >> virt_viewer_events_find_handle(int watch) > > any reason for not keeping the guint *idx arg added in the source > commit? Yeah, as you mentioned, I squashed https://libvirt.org/git/?p=libvirt-glib.git;a=commitdiff;h=1fb34633ef3b318ea678b775d5e47debc98d2184 > >> { >> - unsigned int i; >> - for (i = 0 ; i < nhandles ; i++) >> - if (handles[i]->watch == watch) >> - return handles[i]; >> + guint i; >> + >> + g_return_val_if_fail(handles != NULL, NULL); >> + >> + for (i = 0; i < handles->len; i++) { >> + struct virt_viewer_events_handle *h = g_ptr_array_index(handles, i); >> + >> + if (h == NULL) { >> + g_warn_if_reached(); >> + continue; >> + } >> + >> + if ((h->watch == watch) && !h->removed) { >> + return h; >> + } >> + } >> >> return NULL; >> } >> @@ -182,7 +191,7 @@ virt_viewer_events_cleanup_handle(gpointer user_data) >> if (data->ff) >> (data->ff)(data->opaque); >> >> - free(data); >> + g_ptr_array_remove_fast(handles, data); >> return FALSE; >> } >> >> @@ -199,13 +208,17 @@ virt_viewer_events_remove_handle(int watch) >> >> g_debug("Remove handle %d %d", watch, data->fd); >> >> - if (!data->source) >> - return -1; >> - >> - g_source_remove(data->source); >> - data->source = 0; >> - data->events = 0; >> + if (data->source) { >> + 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 befure it's fully deleted, that's why we need to mar it as > > 'before' 'mark' > > >> + * 'removed' to prevent reuse >> + */ >> + data->removed = TRUE; >> g_idle_add(virt_viewer_events_cleanup_handle, data); >> return 0; >> } >> @@ -214,6 +227,7 @@ struct virt_viewer_events_timeout >> { >> int timer; >> int interval; >> + int removed; >> guint source; >> virEventTimeoutCallback cb; >> void *opaque; >> @@ -222,8 +236,7 @@ struct virt_viewer_events_timeout >> >> >> static int nexttimer = 1; >> -static unsigned int ntimeouts = 0; >> -static struct virt_viewer_events_timeout **timeouts = NULL; >> +GPtrArray *timeouts; > > static > >> >> static gboolean >> virt_viewer_events_dispatch_timeout(void *opaque) >> @@ -243,9 +256,7 @@ virt_viewer_events_add_timeout(int interval, >> { >> struct virt_viewer_events_timeout *data; >> >> - timeouts = g_realloc(timeouts, sizeof(*timeouts)*(ntimeouts+1)); >> - data = g_malloc(sizeof(*data)); >> - memset(data, 0, sizeof(*data)); >> + data = g_new0(struct virt_viewer_events_timeout, 1); >> >> data->timer = nexttimer++; >> data->interval = interval; >> @@ -257,7 +268,7 @@ virt_viewer_events_add_timeout(int interval, >> virt_viewer_events_dispatch_timeout, >> data); >> >> - timeouts[ntimeouts++] = data; >> + g_ptr_array_add(timeouts, data); >> >> g_debug("Add timeout %p %d %p %p %d", data, interval, cb, opaque, data->timer); >> >> @@ -268,10 +279,22 @@ virt_viewer_events_add_timeout(int interval, >> static struct virt_viewer_events_timeout * >> virt_viewer_events_find_timeout(int timer) >> { >> - unsigned int i; >> - for (i = 0 ; i < ntimeouts ; i++) >> - if (timeouts[i]->timer == timer) >> - return timeouts[i]; >> + guint i; >> + >> + g_return_val_if_fail(timeouts != NULL, NULL); >> + >> + for (i = 0; i < timeouts->len; i++) { >> + struct virt_viewer_events_timeout *t = g_ptr_array_index(timeouts, i); >> + >> + if (t == NULL) { >> + g_warn_if_reached(); >> + continue; >> + } >> + >> + if ((t->timer == timer) && !t->removed) { >> + return t; >> + } >> + } >> >> return NULL; >> } >> @@ -319,7 +342,7 @@ virt_viewer_events_cleanup_timeout(gpointer user_data) >> if (data->ff) >> (data->ff)(data->opaque); >> >> - free(data); >> + g_ptr_array_remove_fast(timeouts, data); >> return FALSE; >> } >> >> @@ -336,18 +359,24 @@ virt_viewer_events_remove_timeout(int timer) >> >> g_debug("Remove timeout %p %d", data, timer); >> >> - if (!data->source) >> - return -1; >> - >> - g_source_remove(data->source); >> - data->source = 0; >> + if (data->source) { >> + g_source_remove(data->source); >> + data->source = 0; >> + } >> >> + /* since the actual timeout deletion is done asynchronously, a timeout_update call my >> + * reschedule the timeout before it's fully deleted, that's why we need to mark it as >> + * 'removed' to prevent reuse >> + */ >> + data->removed = TRUE; >> g_idle_add(virt_viewer_events_cleanup_timeout, data); >> return 0; >> } >> >> static gpointer event_register_once(gpointer data G_GNUC_UNUSED) >> { >> + 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, >> virt_viewer_events_update_handle, >> virt_viewer_events_remove_handle, >> -- >> 2.4.4 >> >> _______________________________________________ >> virt-tools-list mailing list >> virt-tools-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/virt-tools-list > > _______________________________________________ > virt-tools-list mailing list > virt-tools-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/virt-tools-list Best Regards, -- Fabiano Fidêncio _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
