[virt-tools-list] [virt-viewer v2 2/6] ovirt: Add OvirtForeignMenu class

[Jonathon Jongsma]

----- Original Message -----
> From: "Jonathon Jongsma" <jjongsma at redhat.com>
> To: "Christophe Fergeau" <cfergeau at redhat.com>
> Cc: virt-tools-list at redhat.com
> Sent: Friday, June 27, 2014 4:42:35 PM
> Subject: Re: [virt-tools-list] [virt-viewer v2 2/6] ovirt: Add OvirtForeignMenu class
> 
> Hi, some comments below
> 
> On Thu, 2014-06-26 at 22:01 +0200, Christophe Fergeau wrote:
> > This class is used to implement the so-called oVirt 'foreign menu'
> > which is a menu populated with ISO images available on the
> > oVirt instance that the user can dynamically insert into the
> > virtual machine he is currently viewing.
> > ---
> >  configure.ac             |   1 +
> >  src/Makefile.am          |   4 +
> >  src/ovirt-foreign-menu.c | 681
> >  +++++++++++++++++++++++++++++++++++++++++++++++
> >  src/ovirt-foreign-menu.h |  82 ++++++
> >  4 files changed, 768 insertions(+)
> >  create mode 100644 src/ovirt-foreign-menu.c
> >  create mode 100644 src/ovirt-foreign-menu.h
> > 
> > diff --git a/configure.ac b/configure.ac
> > index f1b0d1c..8e07c14 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -200,6 +200,7 @@ AS_IF([test "x$have_ovirt" = "xyes"],
> >               [AC_MSG_ERROR([oVirt support requested but libgovirt not
> >               found])
> >        ])
> >  ])
> > +AM_CONDITIONAL([HAVE_OVIRT], [test "x$have_ovirt" = "xyes"])
> >  
> >  dnl Decide if this platform can support the SSH tunnel feature.
> >  AC_CHECK_HEADERS([sys/socket.h sys/un.h windows.h])
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index b3a9637..ee8f885 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -67,6 +67,10 @@ COMMON_SOURCES +=						\
> >  	$(NULL)
> >  endif
> >  
> > +if HAVE_OVIRT
> > +COMMON_SOURCES +=					\
> > +	ovirt-foreign-menu.h ovirt-foreign-menu.c
> > +endif
> >  
> >  if HAVE_LIBVIRT
> >  bin_PROGRAMS += virt-viewer
> > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> > new file mode 100644
> > index 0000000..ab88040
> > --- /dev/null
> > +++ b/src/ovirt-foreign-menu.c
> > @@ -0,0 +1,681 @@
> > +/*
> > + * Virt Viewer: A virtual machine console viewer
> > + *
> > + * Copyright (C) 2007-2014 Red Hat, Inc.
> > + * Copyright (C) 2009-2012 Daniel P. Berrange
> > + * Copyright (C) 2010 Marc-André Lureau
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
> > USA
> > + *
> > + * Author: Daniel P. Berrange <berrange at redhat.com>
> > + *         Christope Fergeau <cfergeau at redhat.com>
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "ovirt-foreign-menu.h"
> > +#include "virt-glib-compat.h"
> > +#include "virt-viewer-util.h"
> > +
> > +typedef enum {
> > +    STATE_0,
> > +    STATE_STORAGE_DOMAIN,
> > +    STATE_VM_CDROM,
> > +    STATE_CDROM_FILE,
> > +    STATE_ISOS
> > +} OvirtForeignMenuState;
> > +
> > +static void ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
> > OvirtForeignMenuState state);
> > +static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu
> > *menu);
> > +static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu
> > *menu);
> > +static void ovirt_foreign_menu_refresh_cdrom_file_async(OvirtForeignMenu
> > *menu);
> > +static gboolean ovirt_foreign_menu_refresh_iso_list(gpointer user_data);
> > +
> > +G_DEFINE_TYPE (OvirtForeignMenu, ovirt_foreign_menu, G_TYPE_OBJECT)
> > +
> > +
> > +struct _OvirtForeignMenuPrivate {
> > +    OvirtProxy *proxy;
> > +    OvirtApi *api;
> > +    OvirtVm *vm;
> > +
> > +    OvirtCollection *files;
> > +    OvirtCdrom *cdrom;
> > +    /* The next 2 members are used when changing the ISO image shown in
> > + *   * a VM */
> > +    /* Menu item for the ISO which is currently used by the VM OvirtCdrom
> > */
> > +    GtkMenuItem *current_iso_menuitem;
> > +    /* Menu item for the ISO we are trying to insert in the VM OvirtCdrom
> > */
> > +    GtkMenuItem *next_iso_menuitem;
> 
> 
> Something seems wrong about storing GtkMenuItem objects here.
> OvirtForeignMenu is owned by the application, so there's a single
> OvirtForeignMenu object for the entire application. However, each
> display window has its own GtkMenu representing this foreign menu. So
> 'current_iso_menuitem' is a pointer to a single menu item from a single
> window. The intent of storing these two values seems to be so that we
> can keep the check-mark on the old menu item until we get notified that
> ovirt has changed the CD.  However, if the 'current_iso_menuitem' refers
> to a menu item in a different window than new menuitem you just clicked,
> it won't work as expected.
> 
> In addition, it appears that we re-create and replace these GtkMenus any
> time the foreign menu 'file' or 'files' properties change. At first I
> was concerned that this would lead to us holding weak references to a
> destroyed GtkMenuItem, but luckily we avoid this issue because:
>  - The act of creating the GtkMenu also updates the
> 'current_iso_menuitem' variable. So a stale 'current_iso_menuitem'
> automatically gets overwritten with a valid one when we create the new
> GtkMenu.
> - The next_iso_menuitem variable does not get updated by creating a new
> GtkMenu, so it could theoretically become invalid. But when
> updated_cdrom_cb() is called, we copy this value to current_iso_menuitem
> just before notifying the 'file' property. This notification causes us
> to re-create the GtkMenu, which overwrites this (invalid)
> current_iso_menuitem value that we just wrote.
> 
> (note that as it is currently implemented, current_iso_menuitem will
> almost always hold the menuitem from the last window, which may not even
> be displayed).
> 
> The rest of the patch looks OK.
> 


I should add that the above analysis was done while looking at the code with all of these patches applied, so some of my comments deal with code that was introduced after this particular patch.  I don't plan to send reviews of the remaining individual patches.  Aside from this particular issue, the rest of the code looks good.

Jonathon