Hey,
----- Mail original -----
>
>
> ----- Original Message -----
> > From: "Christophe Fergeau" <cfergeau@xxxxxxxxxx>
> > To: virt-tools-list@xxxxxxxxxx
> > Sent: Wednesday, April 16, 2014 11:59:53 AM
> > Subject: [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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
