[virt-tools-list] [virt-viewer][PATCH 2/6] events: remove timeout and handle from arrays
Fabiano Fidêncio
fabiano at fidencio.org
Fri Jul 17 16:39:13 UTC 2015
On Fri, Jul 17, 2015 at 6:15 PM, Christophe Fergeau <cfergeau at redhat.com> 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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/virt-tools-list
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
Best Regards,
--
Fabiano Fidêncio
More information about the virt-tools-list
mailing list