[virt-tools-list] [PATCH v2 virt-viewer] App: keep hash table of displays

[Jonathon Jongsma]

On Fri, 2014-09-05 at 00:42 +0200, Fabiano Fidêncio wrote:
> On Thu, 2014-09-04 at 15:34 -0500, Jonathon Jongsma wrote:
> > This is part of a re-factoring that will de-couple the client window
> > from the remote display id.
> > ---
> > 
> > Changes from Fabiano's review
> > 
> >  src/virt-viewer-app.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index b60ce2d..a0b1d76 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -109,6 +109,7 @@ struct _VirtViewerAppPrivate {
> >      VirtViewerWindow *main_window;
> >      GtkWidget *main_notebook;
> >      GHashTable *windows;
> > +    GHashTable *displays;
> >      GHashTable *initial_display_map;
> >      gchar *clipboard;
> >  
> > @@ -934,8 +935,14 @@ virt_viewer_app_display_added(VirtViewerSession *session G_GNUC_UNUSED,
> >      VirtViewerAppPrivate *priv = self->priv;
> >      VirtViewerWindow *window;
> >      gint nth;
> > +    gpointer key = NULL;
> 
> No need to null-ify it here, probably covscan would complain about that.

Hmm. Does coverity really complain about this? I find that very strings.
Many coding standards actually require all variables to be initialized
when they are defined, and this is my habit as well. (Perhaps this is
more common in the C++ world?) It seems to be a particularly good habit
to ensure that pointer variables are initialized, since using a NULL
pointer will generally cause a segfault that is easy to debug, whereas
using an uninitialized pointer will result in non-deterministic behavior
and/or memory corruption. If coverity does complain about this, is there
a way we can disable that particular warning? It seems like a useless
warning.

> >  
> >      g_object_get(display, "nth-display", &nth, NULL);
> > +
> > +    key = GINT_TO_POINTER(nth);
> > +    g_debug("Insert display %d %p", nth, display);
> > +    g_hash_table_insert(self->priv->displays, key, g_object_ref(display));
> 
> Actually, I would not even declare key, I would just do:
> g_hash_table_insert(self->priv->displays, GINT_TO_POINTER(nth),
> g_object_ref(display));
> 
> But it's up to you :-)
> 
> > +
> >      window = virt_viewer_app_get_nth_window(self, nth);
> >      if (window == NULL) {
> >          if (priv->kiosk) {
> > @@ -965,6 +972,7 @@ virt_viewer_app_display_removed(VirtViewerSession *session G_GNUC_UNUSED,
> >  
> >      gtk_widget_hide(GTK_WIDGET(display));
> >      g_object_get(display, "nth-display", &nth, NULL);
> > +    g_hash_table_remove(self->priv->displays, GINT_TO_POINTER(nth));
> >      win = virt_viewer_app_get_nth_window(self, nth);
> >      if (!win)
> >          return;
> > @@ -1636,6 +1644,15 @@ virt_viewer_app_dispose (GObject *object)
> >          g_hash_table_unref(tmp);
> >      }
> >  
> > +    if (priv->displays) {
> > +        GHashTable *tmp = priv->displays;
> > +        /* null-ify before unrefing, because we need
> > +         * to prevent callbacks using priv->displays
> > +         * while it is being disposed of. */
> > +        priv->displays = NULL;
> > +        g_hash_table_unref(tmp);
> > +    }
> > +
> >      g_clear_object(&priv->session);
> >      g_free(priv->title);
> >      priv->title = NULL;
> > @@ -1702,6 +1719,7 @@ virt_viewer_app_init (VirtViewerApp *self)
> >      virt_viewer_app_set_debug(opt_debug);
> >  
> >      self->priv = GET_PRIVATE(self);
> > +    self->priv->displays = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_object_unref);
> >      self->priv->windows = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_object_unref);
> >      self->priv->config = g_key_file_new();
> >      self->priv->config_file = g_build_filename(g_get_user_config_dir(),
> 
> ACK!
>