Thanks for the review and suggestions.
I like the use of the GError as you described, it's definitely a cleaner solution.
Pavel
>
> On Wed, 2014-09-24 at 14:28 -0500, Jonathon Jongsma wrote:
> > Getting close. Comments below.
>
>
> Oh, I forgot to mention one additional thing:
>
> The virt-viewer-vm-connection.xml file needs to be added to
> src/Makefile.am so that it will get installed properly.
>
>
> >
> >
> > On Tue, 2014-09-23 at 14:02 +0200, Pavel Grunt wrote:
> > > When a user tries to connect to ovirt without specifying
> > > VM name (remote-viewer ovirt://ovirt.example.com) or with
> > > wrong VM name a list of available virtual machines is shown,
> > > and the user may pick a machine he wants to connect to.
> > > ---
> > > changes:
> > > - dialog was redesigned
> > > - cancelling the dialog terminates the program
> > > src/remote-viewer.c | 143
> > > +++++++++++++++++++++++++++++++++++---
> > > src/virt-viewer-vm-connection.xml | 138
> > > ++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 271 insertions(+), 10 deletions(-)
> > > create mode 100644 src/virt-viewer-vm-connection.xml
> > >
> > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> > > index a813480..89c96fb 100644
> > > --- a/src/remote-viewer.c
> > > +++ b/src/remote-viewer.c
> > > @@ -74,6 +74,10 @@ enum {
> > > PROP_OPEN_RECENT_DIALOG
> > > };
> > >
> > > +#ifdef HAVE_OVIRT
> > > +static OvirtVm * choose_vm_dialog(char **vm_name,
> > > OvirtCollection *vms, gboolean *any_vm_running);
> > > +#endif
> > > +
> > > static gboolean remote_viewer_start(VirtViewerApp *self);
> > > #ifdef HAVE_SPICE_GTK
> > > static gboolean remote_viewer_activate(VirtViewerApp *self,
> > > GError **error);
> > > @@ -658,11 +662,20 @@ parse_ovirt_uri(const gchar *uri_str, char
> > > **rest_uri, char **name, char **usern
> > > return FALSE;
> > > }
> > >
> > > + if (username && uri->user)
> > > + *username = g_strdup(uri->user);
> > > +
> > > if (uri->path == NULL) {
> > > + *name = NULL;
> > > + *rest_uri = g_strdup_printf("https://%s/api/";,
> > > uri->server);
> > > + xmlFreeURI(uri);
> > > + return TRUE;
> > > + }
> > > +
> > > + if(*uri->path != '/') {
> > > xmlFreeURI(uri);
> > > return FALSE;
> > > }
> > > - g_return_val_if_fail(*uri->path == '/', FALSE);
> > >
> > > /* extract VM name from path */
> > > path_elements = g_strsplit(uri->path, "/", -1);
> > > @@ -676,9 +689,6 @@ parse_ovirt_uri(const gchar *uri_str, char
> > > **rest_uri, char **name, char **usern
> > > vm_name = path_elements[element_count-1];
> > > path_elements[element_count-1] = NULL;
> > >
> > > - if (username && uri->user)
> > > - *username = g_strdup(uri->user);
> > > -
> > > /* build final URI */
> > > rel_path = g_strjoinv("/", path_elements);
> > > /* FIXME: how to decide between http and https? */
> > > @@ -802,7 +812,7 @@
> > > virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
> > >
> > >
> > > static gboolean
> > > -create_ovirt_session(VirtViewerApp *app, const char *uri)
> > > +create_ovirt_session(VirtViewerApp *app, const char *uri,
> > > gboolean *state_choosing)
> >
> > Within this function, there are already quite a few places that use
> > GError to represent various failures. These GErrors are only used
> > within
> > this function and freed before returning. But I wonder if we should
> > return these GErrors to the calling function instead. We'd have to
> > add a
> > new GError type to represent the case where the user canceled the
> > dialog. This would eventually allow the calling function to
> > distinguish
> > between various different error conditions, not just the
> > user-cancellation condition.
> >
> >
> > > {
> > > OvirtProxy *proxy = NULL;
> > > OvirtApi *api = NULL;
> > > @@ -825,6 +835,9 @@ create_ovirt_session(VirtViewerApp *app,
> > > const char *uri)
> > > gchar *ghost = NULL;
> > > gchar *ticket = NULL;
> > > gchar *host_subject = NULL;
> > > + gchar *guid = NULL;
> > > +
> > > + *state_choosing = FALSE;
> > >
> > > g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
> > >
> > > @@ -852,24 +865,43 @@ create_ovirt_session(VirtViewerApp *app,
> > > const char *uri)
> > > g_debug("failed to lookup %s: %s", vm_name,
> > > error->message);
> > > goto error;
> > > }
> > > - vm = OVIRT_VM(ovirt_collection_lookup_resource(vms,
> > > vm_name));
> > > - g_return_val_if_fail(vm != NULL, FALSE);
> > > + if (!vm_name || (vm =
> > > OVIRT_VM(ovirt_collection_lookup_resource(vms, vm_name))) ==
> > > NULL) {
> > > + gboolean any_vm_running;
> > > + *state_choosing = TRUE;
> > > + vm = choose_vm_dialog(&vm_name, vms, &any_vm_running);
> > > + if (!vm) {
> > > + if (any_vm_running) {
> > > + g_debug("no oVirt VM was chosen");
> > > + success = TRUE;
> >
> > Oh, I see that here you are setting the response of
> > create_ovirt_session
> > to TRUE, even though a session was not actually created. I guess
> > this is
> > the case where the user cancelled the vm selection dialog? This
> > feels
> > wrong to me. I think that the return value should be FALSE here,
> > but we
> > should return enough information to determine whether the failure
> > was
> > due to a user cancellation or not. The GError scheme I described
> > above
> > should allow us to do this.
> >
> > > + }
> > > + else {
> > > + g_debug("no oVirt VM running on %s", rest_uri);
> > > + }
> > > + goto error;
> > > + }
> > > + *state_choosing = FALSE;
> > > + }
> > > g_object_get(G_OBJECT(vm), "state", &state, NULL);
> > > if (state != OVIRT_VM_STATE_UP) {
> > > g_debug("oVirt VM %s is not running", vm_name);
> > > goto error;
> > > }
> > > + g_object_set(app, "guest-name", vm_name, NULL);
> > >
> > > if (!ovirt_vm_get_ticket(vm, proxy, &error)) {
> > > g_debug("failed to get ticket for %s: %s", vm_name,
> > > error->message);
> > > goto error;
> > > }
> > >
> > > - g_object_get(G_OBJECT(vm), "display", &display, NULL);
> > > + g_object_get(G_OBJECT(vm), "display", &display, "guid",
> > > &guid, NULL);
> > > if (display == NULL) {
> > > goto error;
> > > }
> > >
> > > + if (guid != NULL) {
> > > + g_object_set(app, "uuid", guid, NULL);
> > > + }
> > > +
> > > g_object_get(G_OBJECT(display),
> > > "type", &type,
> > > "address", &ghost,
> > > @@ -933,6 +965,7 @@ error:
> > > g_free(gtlsport);
> > > g_free(ghost);
> > > g_free(host_subject);
> > > + g_free(guid);
> > >
> > > if (error != NULL)
> > > g_error_free(error);
> > > @@ -1105,6 +1138,87 @@ connect_dialog(gchar **uri)
> > > return retval;
> > > }
> > >
> > > +
> > > +#ifdef HAVE_OVIRT
> > > +static void
> > > +treeview_row_activated_cb(GtkTreeView *treeview G_GNUC_UNUSED,
> > > + GtkTreePath *path G_GNUC_UNUSED,
> > > + GtkTreeViewColumn *col G_GNUC_UNUSED,
> > > + gpointer userdata)
> > > +{
> > > + gtk_widget_activate(GTK_WIDGET(userdata));
> > > +}
> > > +
> > > +static void
> > > +treeselection_changed_cb(GtkTreeSelection *selection, gpointer
> > > userdata)
> > > +{
> > > + gtk_widget_set_sensitive(GTK_WIDGET(userdata),
> > > +
> > > gtk_tree_selection_count_selected_rows(selection)
> > > == 1);
> > > +}
> > > +
> > > +
> > > +static OvirtVm *
> > > +choose_vm_dialog(char **vm_name, OvirtCollection
> > > *vms_collection, gboolean *any_vm_running)
> > > +{
> > > + GtkBuilder *vm_connection;
> > > + GtkWidget *dialog;
> > > + GtkButton *button_connect;
> > > + GtkTreeView *treeview;
> > > + GtkTreeModel *store;
> > > + GtkTreeSelection *select;
> > > + GtkTreeIter iter;
> > > + GHashTable *vms;
> > > + GHashTableIter vms_iter;
> > > + OvirtVm *vm;
> > > + OvirtVmState state;
> > > + int response;
> > > +
> > > + *any_vm_running = FALSE;
> > > + vms = ovirt_collection_get_resources(vms_collection);
> > > +
> > > + vm_connection =
> > > virt_viewer_util_load_ui("virt-viewer-vm-connection.xml");
> > > + dialog = GTK_WIDGET(gtk_builder_get_object(vm_connection,
> > > "vm-connection-dialog"));
> > > + button_connect =
> > > GTK_BUTTON(gtk_builder_get_object(vm_connection,
> > > "button-connect"));
> > > + treeview =
> > > GTK_TREE_VIEW(gtk_builder_get_object(vm_connection,
> > > "treeview"));
> > > + select =
> > > GTK_TREE_SELECTION(gtk_builder_get_object(vm_connection,
> > > "treeview-selection"));
> > > + store = GTK_TREE_MODEL(gtk_builder_get_object(vm_connection,
> > > "store"));
> > > +
> > > + g_signal_connect(treeview, "row-activated",
> > > + G_CALLBACK(treeview_row_activated_cb), button_connect);
> > > + g_signal_connect(select, "changed",
> > > + G_CALLBACK(treeselection_changed_cb), button_connect);
> > > +
> > > + g_hash_table_iter_init(&vms_iter, vms);
> > > + while (g_hash_table_iter_next(&vms_iter, (gpointer *)
> > > vm_name, (gpointer *) &vm)) {
> > > + g_object_get(G_OBJECT(vm), "state", &state, NULL);
> > > + if (state == OVIRT_VM_STATE_UP) {
> > > + gtk_list_store_append(GTK_LIST_STORE(store), &iter);
> > > + gtk_list_store_set(GTK_LIST_STORE(store), &iter, 0,
> > > *vm_name, -1);
> > > + *any_vm_running = TRUE;
> > > + }
> > > + }
> > > + *vm_name = NULL;
> > > + if (*any_vm_running) {
> > > + gtk_widget_show_all(dialog);
> > > + response = gtk_dialog_run(GTK_DIALOG(dialog));
> > > + gtk_widget_hide(dialog);
> > > + if (response == GTK_RESPONSE_ACCEPT &&
> > > + gtk_tree_selection_get_selected(select, &store,
> > > &iter)) {
> > > + gtk_tree_model_get(store, &iter, 0, vm_name, -1);
> > > + vm =
> > > OVIRT_VM(ovirt_collection_lookup_resource(vms_collection,
> > > *vm_name));
> > > + } else {
> > > + vm = NULL;
> > > + }
> > > + } else {
> > > + vm = NULL;
> > > + }
> > > + gtk_widget_destroy(dialog);
> > > + g_object_unref(G_OBJECT(vm_connection));
> > > +
> > > + return vm;
> > > +}
> > > +#endif
> > > +
> > > static gboolean
> > > remote_viewer_start(VirtViewerApp *app)
> > > {
> > > @@ -1171,10 +1285,19 @@ retry_dialog:
> > > }
> > > #ifdef HAVE_OVIRT
> > > if (g_strcmp0(type, "ovirt") == 0) {
> > > - if (!create_ovirt_session(app, guri)) {
> > > - virt_viewer_app_simple_message_dialog(app,
> > > _("Couldn't open oVirt session"));
> > > + gboolean state_choosing;
> > > + if (!create_ovirt_session(app, guri,
> > > &state_choosing)) {
> > > + if (state_choosing) {
> > > + virt_viewer_app_simple_message_dialog(app,
> > > +
> > > _("oVirt
> > > VM is not running on %s"), guri);
> >
> > 'guri' is a user-supplied string, so you have to be careful about
> > using
> > printf formatting. For example, if guri includes a username
> > containing
> > a url-escaped '@' character (%40) followed by a domain starting
> > with the
> > letter 's' (let's say: spice-space.org), this message produces the
> > following amusingly-duplicated message:
> >
> >
> > oVirt VM is not running on ovirt://<USERNAME>oVirt VM is not
> > running on
> > ovirt://<USERNAME>%
> > 40spice-space.org@<OVIRT-SERVER>pice-space.org@<OVIRT-SERVER>
> >
> > As for the message itself, I think something like "No running
> > virtual
> > machines found on <server>" might be more clear.
> >
> >
> > > + } else {
> > > + virt_viewer_app_simple_message_dialog(app,
> > > _("Couldn't open oVirt session"));
> > > + }
> > > + goto cleanup;
> > > + } else if (state_choosing) {
> > > goto cleanup;
> >
> > This logic is a bit confusing. Again, I think it would be cleaner
> > if the
> > GError approach mentioned above was taken so that you could handle
> > all
> > of the failure conditions within the (!create_ovirt_session(...))
> > branch
> > rather than having an additional 'success' case that wasn't really
> > a
> > success.
> >
> > > }
> > > +
> > > } else
> > > #endif
> > > {
> > > diff --git a/src/virt-viewer-vm-connection.xml
> > > b/src/virt-viewer-vm-connection.xml
> > > new file mode 100644
> > > index 0000000..b661df4
> > > --- /dev/null
> > > +++ b/src/virt-viewer-vm-connection.xml
> > > @@ -0,0 +1,138 @@
> > > +<?xml version="1.0" encoding="UTF-8"?>
> > > +<!-- Generated with glade 3.16.1 -->
> > > +<interface>
> > > + <object class="GtkListStore" id="store">
> > > + <columns>
> > > + <!-- column-name name -->
> > > + <column type="gchararray"/>
> > > + <!-- column-name state -->
> > > + <column type="gchararray"/>
> > > + </columns>
> > > + </object>
> > > + <object class="GtkDialog" id="vm-connection-dialog">
> > > + <property name="can_focus">False</property>
> > > + <property name="border_width">5</property>
> > > + <property name="title" translatable="yes">Connection
> > > details</property>
> >
> > The dialog looks much better. I suspect that this title may have
> > been
> > accidentally copied from the remote-viewer dialog? It probably
> > should be
> > more like "Choose a virtual machine" or "Available virtual
> > machines" or
> > something. Can we also give the window a default-height property
> > (maybe
> > 200?) so that it won't be very tiny when there are only 1 or 2 vms
> > running? We should also set the 'expand' property on the label to
> > False
> > so that when the dialog is resized, the treeview will take up all
> > of the
> > extra space rather than splitting it between the treeview and
> > label.
> >
> > > + <property name="modal">True</property>
> > > + <property name="window_position">center-on-parent</property>
> > > + <property name="destroy_with_parent">True</property>
> > > + <property name="type_hint">dialog</property>
> > > + <property name="skip_taskbar_hint">True</property>
> > > + <property name="skip_pager_hint">True</property>
> > > + <child internal-child="vbox">
> > > + <object class="GtkBox" id="dialog-vbox1">
> > > + <property name="can_focus">False</property>
> > > + <property name="orientation">vertical</property>
> > > + <property name="spacing">6</property>
> > > + <child internal-child="action_area">
> > > + <object class="GtkButtonBox" id="dialog-action_area1">
> > > + <property name="can_focus">False</property>
> > > + <property name="layout_style">end</property>
> > > + <child>
> > > + <object class="GtkButton" id="button-cancel">
> > > + <property name="label">gtk-cancel</property>
> > > + <property name="visible">True</property>
> > > + <property name="can_focus">True</property>
> > > + <property
> > > name="receives_default">True</property>
> > > + <property name="use_stock">True</property>
> > > + </object>
> > > + <packing>
> > > + <property name="expand">False</property>
> > > + <property name="fill">True</property>
> > > + <property name="position">0</property>
> > > + </packing>
> > > + </child>
> > > + <child>
> > > + <object class="GtkButton" id="button-connect">
> > > + <property name="label">gtk-connect</property>
> > > + <property name="visible">True</property>
> > > + <property name="can_focus">True</property>
> > > + <property name="can_default">True</property>
> > > + <property name="has_default">True</property>
> > > + <property
> > > name="receives_default">True</property>
> > > + <property name="use_stock">True</property>
> > > + </object>
> > > + <packing>
> > > + <property name="expand">False</property>
> > > + <property name="fill">True</property>
> > > + <property name="position">1</property>
> > > + </packing>
> > > + </child>
> > > + </object>
> > > + <packing>
> > > + <property name="expand">False</property>
> > > + <property name="fill">True</property>
> > > + <property name="pack_type">end</property>
> > > + <property name="position">0</property>
> > > + </packing>
> > > + </child>
> > > + <child>
> > > + <object class="GtkTreeView" id="treeview">
> > > + <property name="visible">True</property>
> > > + <property name="can_focus">True</property>
> > > + <property name="model">store</property>
> > > + <property name="headers_visible">False</property>
> > > + <property name="search_column">0</property>
> > > + <property
> > > name="enable_grid_lines">horizontal</property>
> > > + <child internal-child="selection">
> > > + <object class="GtkTreeSelection"
> > > id="treeview-selection"/>
> > > + </child>
> > > + <child>
> > > + <object class="GtkTreeViewColumn"
> > > id="treeviewcolumn1">
> > > + <property name="title"
> > > translatable="yes">Name</property>
> > > + <child>
> > > + <object class="GtkCellRendererText"
> > > id="cellrenderertext1"/>
> > > + <attributes>
> > > + <attribute name="text">0</attribute>
> > > + </attributes>
> > > + </child>
> > > + </object>
> > > + </child>
> > > + <child>
> > > + <object class="GtkTreeViewColumn"
> > > id="treeviewcolumn2">
> > > + <property name="title"
> > > translatable="yes">State</property>
> > > + <child>
> > > + <object class="GtkCellRendererText"
> > > id="cellrenderertext2"/>
> > > + <attributes>
> > > + <attribute name="text">1</attribute>
> > > + </attributes>
> > > + </child>
> > > + </object>
> > > + </child>
> > > + </object>
> > > + <packing>
> > > + <property name="expand">True</property>
> > > + <property name="fill">True</property>
> > > + <property name="pack_type">end</property>
> > > + <property name="position">1</property>
> > > + </packing>
> > > + </child>
> > > + <child>
> > > + <object class="GtkLabel" id="label">
> > > + <property name="visible">True</property>
> > > + <property name="can_focus">False</property>
> > > + <property name="xalign">0</property>
> > > + <property name="yalign">0</property>
> > > + <property name="xpad">4</property>
> > > + <property name="label" translatable="yes">Available
> > > virtual machines</property>
> > > + <property name="ellipsize">end</property>
> > > + <property name="width_chars">26</property>
> > > + <attributes>
> > > + <attribute name="weight" value="bold"/>
> > > + </attributes>
> > > + </object>
> > > + <packing>
> > > + <property name="expand">True</property>
> > > + <property name="fill">True</property>
> > > + <property name="pack_type">end</property>
> > > + <property name="position">2</property>
> > > + </packing>
> > > + </child>
> > > + </object>
> > > + </child>
> > > + <action-widgets>
> > > + <action-widget response="-6">button-cancel</action-widget>
> > > + <action-widget
> > > response="-3">button-connect</action-widget>
> > > + </action-widgets>
> > > + </object>
> > > +</interface>
> >
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list@xxxxxxxxxx
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
>
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
>
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
