Re: [virt-tools-list] [virt-viewer][PATCH 2/6] events: remove timeout and handle from arrays

2015-07-20 Thread Christophe Fergeau
On Fri, Jul 17, 2015 at 06:39:13PM +0200, Fabiano Fidêncio wrote:
 On Fri, Jul 17, 2015 at 6:15 PM, Christophe Fergeau cferg...@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 cp the initial log too
 
 Not sure if the initial log would make sense here. But I can cp 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.

It makes reviewing much more mechanical if each of your patch corresponds
to exactly one libvirt-glib commit. At the very least, mention all
commits that were squashed together in the commit log, it's very
misleading as you did it currently, I did not initially realize several
commits were squashed and I was wondering why you needed to do these
changes compared to the initial commit.

Christophe


pgpdgzKmtvBvI.pgp
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list

Re: [virt-tools-list] [virt-viewer][PATCH 2/6] events: remove timeout and handle from arrays

2015-07-17 Thread Christophe Fergeau
On Fri, Jul 17, 2015 at 06:15:19PM +0200, Christophe Fergeau wrote:
  @@ -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?
 

Ah ok you squashed
https://libvirt.org/git/?p=libvirt-glib.git;a=commitdiff;h=1fb34633ef3b318ea678b775d5e47debc98d2184
in

Christophe


pgpIW0sxKMHav.pgp
Description: PGP signature
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list

Re: [virt-tools-list] [virt-viewer][PATCH 2/6] events: remove timeout and handle from arrays

2015-07-17 Thread Fabiano Fidêncio
On Fri, Jul 17, 2015 at 6:15 PM, Christophe Fergeau cferg...@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 cp the initial log too

Not sure if the initial log would make sense here. But I can cp 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++;