[virt-tools-list] [virt-viewer 6/7] Implement ovirt_foreign_menu_new_from_file()

[Christophe Fergeau]

Hey,

----- Mail original -----
> 
> 
> ----- Original Message -----
> > From: "Christophe Fergeau" <cfergeau at redhat.com>
> > To: virt-tools-list at redhat.com
> > Sent: Wednesday, April 16, 2014 11:59:53 AM
> > Subject: [virt-tools-list] [virt-viewer 6/7] Implement
> > 	ovirt_foreign_menu_new_from_file()
> > 
> > This will create an OvirtForeignMenu instance from a .vv file. Contrary to
> > the ovirt:// case when we already have an OvirtAPI and OvirtVm instance,
> > when working from a .vv file, we'll need to get them from the REST API.
> > Authentication should happen through the JSESSIONID cookie, if that fails
> > we want to give up on using the foreign menu, so we don't need to set up
> > authentication callbacks.
> > ---
> >  src/ovirt-foreign-menu.c | 174
> >  +++++++++++++++++++++++++++++++++++++++++++++++
> >  src/ovirt-foreign-menu.h |   2 +
> >  2 files changed, 176 insertions(+)
> > 
> > diff --git a/src/ovirt-foreign-menu.c b/src/ovirt-foreign-menu.c
> > index 642c0ef..57ffd60 100644
> > --- a/src/ovirt-foreign-menu.c
> > +++ b/src/ovirt-foreign-menu.c
> > @@ -25,6 +25,8 @@
> >  
> >  #include <config.h>
> >  
> > +#include <string.h>
> > +
> >  #include "ovirt-foreign-menu.h"
> >  #include "virt-glib-compat.h"
> >  
> > @@ -35,12 +37,16 @@
> >  
> >  typedef enum {
> >      STATE_0,
> > +    STATE_API,
> > +    STATE_VM,
> >      STATE_STORAGE_DOMAIN,
> >      STATE_VM_CDROM,
> >      STATE_ISOS
> >  } OvirtForeignMenuState;
> >  
> >  static void ovirt_foreign_menu_next_async_step(OvirtForeignMenu *menu,
> >  OvirtForeignMenuState state);
> > +static void ovirt_foreign_menu_fetch_api_async(OvirtForeignMenu *menu);
> > +static void ovirt_foreign_menu_fetch_vm_async(OvirtForeignMenu *menu);
> >  static void ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu
> >  *menu);
> >  static void ovirt_foreign_menu_fetch_vm_cdrom_async(OvirtForeignMenu
> >  *menu);
> >  static gboolean ovirt_foreign_menu_refresh_iso_list(gpointer user_data);
> > @@ -52,11 +58,13 @@ struct _OvirtForeignMenuPrivate {
> >      OvirtProxy *proxy;
> >      OvirtApi *api;
> >      OvirtVm *vm;
> > +    char *vm_guid;
> >  
> >      OvirtCollection *files;
> >      OvirtCdrom *cdrom;
> >  
> >      GList *iso_names;
> > +
> >  };
> >  
> >  
> > @@ -69,6 +77,7 @@ enum {
> >      PROP_API,
> >      PROP_VM,
> >      PROP_FILES,
> > +    PROP_VM_GUID,
> >  };
> >  
> >  static void
> > @@ -90,6 +99,10 @@ ovirt_foreign_menu_get_property(GObject *object, guint
> > property_id,
> >          break;
> >      case PROP_FILES:
> >          g_value_set_pointer(value, priv->iso_names);
> > +        break;
> 
> Looks like there was a missing break here before?  I guess it's not a huge
> deal, since you'll just get an extraneous invalid property warning when you
> get the 'files' property, but it might be worth fixing that in the patch 2
> where you introduced this class.

Ah right, I've moved it to the right place.

> 
> > +    case PROP_VM_GUID:
> > +        g_value_set_string(value, priv->vm_guid);
> > +        break;
> >  
> >      default:
> >          G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
> > @@ -122,6 +135,15 @@ ovirt_foreign_menu_set_property(GObject *object, guint
> > property_id,
> >              g_object_unref(priv->vm);
> >          }
> >          priv->vm = g_value_dup_object(value);
> > +        g_free(priv->vm_guid);
> > +        priv->vm_guid = NULL;
> > +        if (priv->vm != NULL) {
> > +            g_object_get(G_OBJECT(priv->vm), "guid", &priv->vm_guid,
> > NULL);
> > +        }
> > +        break;
> > +    case PROP_VM_GUID:
> > +        g_free(priv->vm_guid);
> > +        priv->vm_guid = g_value_dup_string(value);
> 
> Is it valid to set the vm-guid property when priv->vm is non-null?  Should we
> issue a warning if priv->vm is already set?

I've made vm-guid as CONSTRUCT-ONLY, and I've made setting the vm-guid property
unset priv->vm

> > ovirt_foreign_menu_fetch_storage_domain_async(OvirtForeignMenu *menu
> >  }
> >  
> >  
> > +static void vms_fetched_cb(GObject *source_object,
> > +                           GAsyncResult *result,
> > +                           gpointer user_data)
> > +{
> > +    GError *error = NULL;
> > +    OvirtForeignMenu *menu = OVIRT_FOREIGN_MENU(user_data);
> > +    OvirtCollection *collection;
> > +    GList *vms;
> > +    GList *it;
> > +
> > +    collection = OVIRT_COLLECTION(source_object);
> > +    ovirt_collection_fetch_finish(collection, result, &error);
> > +    if (error != NULL) {
> > +        g_debug("failed to fetch VM list: %s", error->message);
> > +        g_clear_error(&error);
> > +        return;
> > +    }
> > +
> > +    vms =
> > g_hash_table_get_values(ovirt_collection_get_resources(collection));
> 
> GHashTableIter?
> 
> > +    for (it = vms; it != NULL; it = it->next) {
> > +        char *guid;
> > +
> > +        g_object_get(G_OBJECT(it), "guid", &guid, NULL);
> 
> 'it' is GList*, I assume you want it->data (assuming you don't change to
> GHashTableIter...)

Yes, thanks, I actually remembered that the 'foreign menu from .vv file' codepaths were untested, which explain this kind of bugs...  I will send a v2 with this code actually tested.
> 
> > +        if (g_strcmp0(guid, menu->priv->vm_guid) == 0) {
> > +            menu->priv->vm = g_object_ref(it);
> 
> same issue here.  Also, I assume you can simply break out of the loop here
> after you find the matching GUID?
> 

Yup, added this break.

> > +        }
> > +    }
> > +    g_list_free(vms);
> > +    if (menu->priv->vm != NULL) {
> > +        ovirt_foreign_menu_next_async_step(menu, STATE_VM);
> > +    } else {
> > +        g_debug("failed to find a VM with guid \"%s\"",
> > menu->priv->vm_guid);
> 
> warning?

Added too.


> > @@ -578,3 +709,46 @@ static gboolean
> > ovirt_foreign_menu_refresh_iso_list(gpointer user_data)
> >       */
> >      return G_SOURCE_REMOVE;
> >  }
> > +
> > +
> > +OvirtForeignMenu *ovirt_foreign_menu_new_from_file(VirtViewerFile *file)
> > +{
> > +    OvirtProxy *proxy = NULL;
> > +    OvirtForeignMenu *menu = NULL;
> > +    char *ca_str = NULL;
> > +    char *jsessionid = NULL;
> > +    char *url = NULL;
> > +    char *vm_guid = NULL;
> > +    GByteArray *ca = NULL;
> > +
> > +    url = virt_viewer_file_get_ovirt_host(file);
> > +    vm_guid = virt_viewer_file_get_ovirt_vm_guid(file);
> > +    jsessionid = virt_viewer_file_get_ovirt_jsessionid(file);
> > +    ca_str = virt_viewer_file_get_ovirt_ca(file);
> > +
> > +    if ((url == NULL) || (vm_guid == NULL))
> > +        goto end;
> > +
> > +    proxy = ovirt_proxy_new(url);
> > +    if (proxy == NULL)
> > +        goto end;
> > +
> > +    if (ca_str != NULL) {
> > +        ca = g_byte_array_new_take((guint8 *)ca_str, strlen(ca_str) + 1);
> > +        ca_str = NULL;
> > +    }
> > +
> > +    g_object_set(G_OBJECT(proxy),
> > +                 "session-id", jsessionid,
> > +                 "ca-cert", ca,
> > +                 NULL);
> > +    menu = ovirt_foreign_menu_new(proxy);
> > +    g_object_set(G_OBJECT(menu), "guid", vm_guid, NULL);
> > +end:
> > +    g_free(url);
> > +    g_free(vm_guid);
> > +    g_free(jsessionid);
> > +    g_free(ca_str);
> 
> 'ca' byte array is leaked?

Yup, thanks for catching that!

Christophe