[virt-tools-list] [PATCH v5 remote-viewer] Connecting to ovirt without specifying VM name will not fail
Jonathon Jongsma
jjongsma at redhat.com
Fri Sep 26 15:45:40 UTC 2014
Great, thanks for iterating this patch so many times. Looks good now.
You might consider changing the error code to something slightly more
generic so that it could be used in other places (e.g. something like
VIRT_VIEWER_ERROR_USER_CANCELLED instead of
VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED), but that's up to you.
ACK.
(interestingly, I note that my spellchecker wants to use 'canceled' with
one L rather than 'cancelled' with 2 Ls. There seems to be some debate
on this issue: http://grammarist.com/spelling/cancel/. Personally I
usually use 2 Ls).
On Fri, 2014-09-26 at 15:55 +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.
> ---
> v5:
> - GError is set for all failures
> v4:
> - new GError types were added for handling the cancellation of the dialog
> - changed title and default height of the dialog
> v3:
> - dialog was redesigned
> - cancelling the dialog terminates the program
>
> src/Makefile.am | 1 +
> src/remote-viewer.c | 176 +++++++++++++++++++++++++++++++++-----
> src/virt-viewer-util.h | 1 +
> src/virt-viewer-vm-connection.xml | 139 ++++++++++++++++++++++++++++++
> 4 files changed, 295 insertions(+), 22 deletions(-)
> create mode 100644 src/virt-viewer-vm-connection.xml
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index c425522..a33417c 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -9,6 +9,7 @@ builderxml_DATA = \
> virt-viewer-about.xml \
> virt-viewer-auth.xml \
> virt-viewer-guest-details.xml \
> + virt-viewer-vm-connection.xml \
> $(NULL)
>
> EXTRA_DIST = \
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 49981aa..3b44269 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, GError **error);
> +#endif
> +
> static gboolean remote_viewer_start(VirtViewerApp *self);
> #ifdef HAVE_SPICE_GTK
> static gboolean remote_viewer_activate(VirtViewerApp *self, GError **error);
> @@ -648,19 +652,27 @@ parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **usern
> g_return_val_if_fail(name != NULL, FALSE);
>
> uri = xmlParseURI(uri_str);
> - if (uri == NULL)
> - return FALSE;
> + g_return_val_if_fail(uri != NULL, FALSE);
>
> if (g_strcmp0(uri->scheme, "ovirt") != 0) {
> xmlFreeURI(uri);
> 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);
> @@ -668,15 +680,11 @@ parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **usern
> element_count = g_strv_length(path_elements);
> if (element_count == 0) {
> g_strfreev(path_elements);
> - xmlFreeURI(uri);
> return FALSE;
> }
> 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? */
> @@ -799,7 +807,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, GError **err)
> {
> OvirtProxy *proxy = NULL;
> OvirtApi *api = NULL;
> @@ -822,15 +830,17 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> gchar *ghost = NULL;
> gchar *ticket = NULL;
> gchar *host_subject = NULL;
> + gchar *guid = NULL;
>
> g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
>
> - if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username))
> - goto error;
> - proxy = ovirt_proxy_new(rest_uri);
> - if (proxy == NULL)
> + if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username)) {
> + g_set_error_literal(&error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> + _("failed to parse ovirt uri"));
> goto error;
> + }
>
> + proxy = ovirt_proxy_new(rest_uri);
> g_object_set(proxy,
> "username", username,
> NULL);
> @@ -849,24 +859,38 @@ 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 == NULL ||
> + (vm = OVIRT_VM(ovirt_collection_lookup_resource(vms, vm_name))) == NULL) {
> + vm = choose_vm_dialog(&vm_name, vms, &error);
> + if (vm == NULL) {
> + goto error;
> + }
> + }
> 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);
> + g_set_error(&error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> + _("oVirt VM %s is not running"), vm_name);
> + g_debug(error->message);
> 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) {
> + g_set_error(&error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> + _("oVirt VM %s has no display"), vm_name);
> goto error;
> }
>
> + if (guid != NULL) {
> + g_object_set(app, "uuid", guid, NULL);
> + }
> +
> g_object_get(G_OBJECT(display),
> "type", &type,
> "address", &ghost,
> @@ -883,7 +907,9 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> } else if (type == OVIRT_VM_DISPLAY_VNC) {
> session_type = "vnc";
> } else {
> - g_debug("Unknown display type: %d", type);
> + g_set_error(&error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> + _("oVirt VM %s has unknown display type: %d"), vm_name, type);
> + g_debug(error->message);
> goto error;
> }
>
> @@ -896,9 +922,11 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> virt_viewer_app_set_connect_info(app, NULL, ghost, gport, gtlsport,
> session_type, NULL, NULL, 0, NULL);
>
> - if (virt_viewer_app_create_session(app, session_type) < 0)
> + if (virt_viewer_app_create_session(app, session_type) < 0) {
> + g_set_error(&error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> + _("Failed to create a session type %s"), session_type);
> goto error;
> -
> + }
> #ifdef HAVE_SPICE_GTK
> if (type == OVIRT_VM_DISPLAY_SPICE) {
> SpiceSession *session;
> @@ -930,9 +958,10 @@ error:
> g_free(gtlsport);
> g_free(ghost);
> g_free(host_subject);
> + g_free(guid);
>
> if (error != NULL)
> - g_error_free(error);
> + g_propagate_error(err, error);
> if (display != NULL)
> g_object_unref(display);
> if (vm != NULL)
> @@ -1102,6 +1131,98 @@ 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, GError **error)
> +{
> + 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;
> + gboolean any_vm_running = FALSE;
> +
> + g_return_val_if_fail(vm_name != NULL, NULL);
> + if (*vm_name != NULL) {
> + free(*vm_name);
> + }
> +
> + 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 {
> + g_set_error_literal(error,
> + VIRT_VIEWER_ERROR, VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED,
> + _("No virtual machine was chosen"));
> + vm = NULL;
> + }
> + } else {
> + g_set_error_literal(error,
> + VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> + _("No running virtual machines found"));
> + vm = NULL;
> + }
> + gtk_widget_destroy(dialog);
> + g_object_unref(G_OBJECT(vm_connection));
> +
> + return vm;
> +}
> +#endif
> +
> static gboolean
> remote_viewer_start(VirtViewerApp *app)
> {
> @@ -1168,8 +1289,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"));
> + if (!create_ovirt_session(app, guri, &error)) {
> + if (error) {
> + if (!g_error_matches(error,
> + VIRT_VIEWER_ERROR,
> + VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED)) {
> + virt_viewer_app_simple_message_dialog(app,
> + _("Couldn't open oVirt session: %s"),
> + error->message);
> + }
> + } else {
> + virt_viewer_app_simple_message_dialog(app, _("Couldn't open oVirt session"));
> + }
> + g_clear_error(&error);
> goto cleanup;
> }
> } else
> diff --git a/src/virt-viewer-util.h b/src/virt-viewer-util.h
> index 287f9bd..dda7b1d 100644
> --- a/src/virt-viewer-util.h
> +++ b/src/virt-viewer-util.h
> @@ -30,6 +30,7 @@ extern gboolean doDebug;
>
> enum {
> VIRT_VIEWER_ERROR_FAILED,
> + VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED,
> };
>
> #define VIRT_VIEWER_ERROR virt_viewer_error_quark ()
> diff --git a/src/virt-viewer-vm-connection.xml b/src/virt-viewer-vm-connection.xml
> new file mode 100644
> index 0000000..147dc92
> --- /dev/null
> +++ b/src/virt-viewer-vm-connection.xml
> @@ -0,0 +1,139 @@
> +<?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">Choose a virtual machine</property>
> + <property name="modal">True</property>
> + <property name="window_position">center-on-parent</property>
> + <property name="default_height">200</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">False</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>
More information about the virt-tools-list
mailing list