[virt-tools-list] [PATCH v4 remote-viewer] Connecting to ovirt without specifying VM name will not fail
Jonathon Jongsma
jjongsma at redhat.com
Thu Sep 25 17:09:52 UTC 2014
Looking good, but a few more comments below.
On Thu, 2014-09-25 at 14:56 +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.
> ---
> 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 | 178 ++++++++++++++++++++++++++++++--------
> src/virt-viewer-util.h | 2 +
> src/virt-viewer-vm-connection.xml | 139 +++++++++++++++++++++++++++++
> 4 files changed, 285 insertions(+), 35 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..e189d47 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);
> @@ -635,31 +639,28 @@ remote_viewer_window_added(VirtViewerApp *app,
>
> #ifdef HAVE_OVIRT
> static gboolean
> -parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **username)
> +process_ovirt_uri(xmlURIPtr uri, char **rest_uri, char **name, char **username)
> {
> char *vm_name = NULL;
> char *rel_path;
> - xmlURIPtr uri;
> gchar **path_elements;
> guint element_count;
>
> - g_return_val_if_fail(uri_str != NULL, FALSE);
> + g_return_val_if_fail(uri != NULL, FALSE);
> g_return_val_if_fail(rest_uri != NULL, FALSE);
> g_return_val_if_fail(name != NULL, FALSE);
>
> - uri = xmlParseURI(uri_str);
> - if (uri == NULL)
> - return FALSE;
> + g_return_val_if_fail(g_strcmp0(uri->scheme, "ovirt") == 0, 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) {
> - xmlFreeURI(uri);
> - return FALSE;
> + *name = NULL;
> + *rest_uri = g_strdup_printf("https://%s/api/", uri->server);
> + return TRUE;
> }
> +
> g_return_val_if_fail(*uri->path == '/', FALSE);
>
> /* extract VM name from path */
> @@ -668,15 +669,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? */
> @@ -684,7 +681,6 @@ parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **usern
> *name = vm_name;
> g_free(rel_path);
> g_strfreev(path_elements);
> - xmlFreeURI(uri);
>
> g_debug("oVirt base URI: %s", *rest_uri);
> g_debug("oVirt VM name: %s", *name);
> @@ -799,7 +795,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 **error)
> {
> OvirtProxy *proxy = NULL;
> OvirtApi *api = NULL;
> @@ -807,7 +803,6 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> OvirtVm *vm = NULL;
> OvirtVmDisplay *display = NULL;
> OvirtVmState state;
> - GError *error = NULL;
Generally, functions that accept a GError** parameter allow the caller
to pass NULL if they're not interested in the error details. So it's
probably better to keep this local GError variable and copy it into the
output argument at the end if it's non-NULL.
> char *rest_uri = NULL;
> char *vm_name = NULL;
> char *username = NULL;
> @@ -822,10 +817,14 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> gchar *ghost = NULL;
> gchar *ticket = NULL;
> gchar *host_subject = NULL;
> + gchar *guid = NULL;
> +
> + xmlURIPtr uri_ptr;
>
> g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), FALSE);
>
> - if (!parse_ovirt_uri(uri, &rest_uri, &vm_name, &username))
> + uri_ptr = xmlParseURI(uri);
> + if (!process_ovirt_uri(uri_ptr, &rest_uri, &vm_name, &username))
> goto error;
Here we are returning FALSE without setting the GError output parameter.
We should set a GError for all failures...
> proxy = ovirt_proxy_new(rest_uri);
> if (proxy == NULL)
This is missing some context in the diff, but we also will return FALSE
without setting a GError in this case. Fortunately, ovirt_proxy_new()
can never fail, so we can simply remove this check.
> @@ -838,35 +837,55 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> g_signal_connect(G_OBJECT(proxy), "authenticate",
> G_CALLBACK(authenticate_cb), app);
>
> - api = ovirt_proxy_fetch_api(proxy, &error);
> - if (error != NULL) {
> - g_debug("failed to get oVirt 'api' collection: %s", error->message);
> + api = ovirt_proxy_fetch_api(proxy, error);
> + if (*error != NULL) {
> + g_debug("failed to get oVirt 'api' collection: %s", (*error)->message);
> goto error;
> }
> vms = ovirt_api_get_vms(api);
> - ovirt_collection_fetch(vms, proxy, &error);
> - if (error != NULL) {
> - g_debug("failed to lookup %s: %s", vm_name, error->message);
> + ovirt_collection_fetch(vms, proxy, error);
> + if (*error != NULL) {
> + 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) {
> + gboolean any_vm_running;
> + vm = choose_vm_dialog(&vm_name, vms, &any_vm_running);
> + if (vm == NULL) {
> + if (any_vm_running) {
> + g_set_error_literal(error,
> + VIRT_VIEWER_ERROR, VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED,
> + _("no oVirt VM was chosen"));
> + } else {
> + g_set_error(error,
> + VIRT_VIEWER_ERROR, VIRT_VIEWER_NO_RUNNING_VMS,
> + _("No running virtual machines found on %s"), uri_ptr->server);
> + }
I think this logic seems more appropriate inside the choose_vm_dialog()
function. Then choose_vm_dialog() would simply accept a GError**
parameter and set it appropriately.
> + 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);
Also missing a GError for this case.
> 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);
> + 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;
and here
> }
>
> + if (guid != NULL) {
> + g_object_set(app, "uuid", guid, NULL);
> + }
> +
> g_object_get(G_OBJECT(display),
> "type", &type,
> "address", &ghost,
There are a couple additional failure cases that are not shown in this
diff for which we don't currently set a GError.
here:
} else {
g_debug("Unknown display type: %d", type);
goto error;
}
and here:
if (virt_viewer_app_create_session(app, session_type) < 0)
goto error;
I don't think that all of these error cases necessarily need their own
error code enumeration. Perhaps you can just use the generic
VIRT_VIEWER_ERROR_FAILED and provide additional details in the message.
It's currently only important that we can distinguish the
dialog-cancelled case from all the others.
> @@ -930,9 +949,10 @@ error:
> g_free(gtlsport);
> g_free(ghost);
> g_free(host_subject);
> + g_free(guid);
> +
> + xmlFreeURI(uri_ptr);
>
> - if (error != NULL)
> - g_error_free(error);
Here is where you could copy the local GError into the output param if
it was non-null.
> if (display != NULL)
> g_object_unref(display);
> if (vm != NULL)
> @@ -1102,6 +1122,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)
> {
> @@ -1168,8 +1269,15 @@ 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 (g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_NO_RUNNING_VMS)) {
> + virt_viewer_app_simple_message_dialog(app, error->message);
> + } else 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"));
> + }
> + g_clear_error(&error);
> goto cleanup;
> }
> } else
> diff --git a/src/virt-viewer-util.h b/src/virt-viewer-util.h
> index 287f9bd..97b3188 100644
> --- a/src/virt-viewer-util.h
> +++ b/src/virt-viewer-util.h
> @@ -30,6 +30,8 @@ extern gboolean doDebug;
>
> enum {
> VIRT_VIEWER_ERROR_FAILED,
> + VIRT_VIEWER_NO_RUNNING_VMS,
> + 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