[virt-tools-list] [PATCH v3 remote-viewer] Connecting to ovirt without specifying VM name will not fail
Jonathon Jongsma
jjongsma at redhat.com
Wed Sep 24 20:27:23 UTC 2014
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
More information about the virt-tools-list
mailing list