Getting close. Comments below.
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
