On Mon, 2014-09-29 at 11:02 +0200, Pavel Grunt wrote: > When user starts virt-viewer without specifying VM domain name > or with a wrong name a list of running machines is shown > and user may choose one of them. > --- > Depends on http://www.redhat.com/archives/virt-tools-list/2014-September/msg00253.html > > src/virt-viewer-main.c | 4 +- > src/virt-viewer.c | 118 +++++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 106 insertions(+), 16 deletions(-) > > diff --git a/src/virt-viewer-main.c b/src/virt-viewer-main.c > index 44d1182..3fae955 100644 > --- a/src/virt-viewer-main.c > +++ b/src/virt-viewer-main.c > @@ -104,12 +104,12 @@ int main(int argc, char **argv) > > g_option_context_free(context); > > - if (!args || (g_strv_length(args) != 1)) { > + if (args && (g_strv_length(args) != 1)) { > g_printerr(_("\nUsage: %s [OPTIONS] DOMAIN-NAME|ID|UUID\n\n%s\n\n"), argv[0], help_msg); > goto cleanup; > } > > - viewer = virt_viewer_new(uri, args[0], direct, attach, waitvm, reconnect); > + viewer = virt_viewer_new(uri, (args) ? args[0] : NULL, direct, attach, waitvm, reconnect); > if (viewer == NULL) > goto cleanup; > > diff --git a/src/virt-viewer.c b/src/virt-viewer.c > index c6066c5..f19163d 100644 > --- a/src/virt-viewer.c > +++ b/src/virt-viewer.c > @@ -526,6 +526,87 @@ virt_viewer_dispose (GObject *object) > G_OBJECT_CLASS(virt_viewer_parent_class)->dispose (object); > } > So, a lot of the following code is almost exactly the same as the code we added to remote-viewer.c for the ovirt case. Have you tried to factor out the common parts so we don't have so much duplication? > +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 virDomainPtr > +choose_vm_dialog(char **vm_name, virConnectPtr conn, GError **error) > +{ > + GtkBuilder *vm_connection; > + GtkWidget *dialog; > + GtkButton *button_connect; > + GtkTreeView *treeview; > + GtkTreeModel *store; > + GtkTreeSelection *select; > + GtkTreeIter iter; > + virDomainPtr *domains, dom = NULL; > + int i, vms_running, response; > + unsigned int flags = VIR_CONNECT_LIST_DOMAINS_RUNNING; > + > + g_return_val_if_fail(vm_name != NULL, NULL); > + if (*vm_name != NULL) { > + free(*vm_name); > + } > + > + 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); > + > + vms_running = virConnectListAllDomains(conn, &domains, flags); > + for (i = 0; i < vms_running; i++) { > + gtk_list_store_append(GTK_LIST_STORE(store), &iter); > + gtk_list_store_set(GTK_LIST_STORE(store), &iter, 0, virDomainGetName(domains[i]), -1); > + virDomainFree(domains[i]); > + } > + free(domains); > + > + if (vms_running > 0) { > + 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); > + dom = virDomainLookupByName(conn, *vm_name); It's unlikely, but theoretically possible that virDomainLookupByName() could return NULL here. The domain may have been destroyed between the time that the dialog was shown and when the user made a selection. In that case, we should probably set a GError here. > + } else { > + g_set_error_literal(error, > + VIRT_VIEWER_ERROR, VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED, > + _("No virtual machine was chosen")); > + } > + } else { > + char *uri = virConnectGetURI(conn); > + g_set_error(error, > + VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED, > + _("No running virtual machines found on %s"), uri); > + free(uri); > + } > + gtk_widget_destroy(dialog); > + g_object_unref(G_OBJECT(vm_connection)); > + > + return dom; > +} > + > static int virt_viewer_connect(VirtViewerApp *app); > > static gboolean > @@ -537,6 +618,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > VirtViewer *self = VIRT_VIEWER(app); > VirtViewerPrivate *priv = self->priv; > char uuid_string[VIR_UUID_STRING_BUFLEN]; > + GError *err = NULL; > > g_debug("initial connect"); > > @@ -547,19 +629,22 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > } > > virt_viewer_app_show_status(app, _("Finding guest domain")); > - dom = virt_viewer_lookup_domain(self); > - if (!dom) { > - if (priv->waitvm) { > - virt_viewer_app_show_status(app, _("Waiting for guest domain to be created")); > - virt_viewer_app_trace(app, "Guest %s does not yet exist, waiting for it to be created", > - priv->domkey); > - goto done; > - } else { > - virt_viewer_app_simple_message_dialog(app, _("Cannot find guest domain %s"), > - priv->domkey); > - g_debug("Cannot find guest %s", priv->domkey); > + > + if (priv->domkey == NULL || I guess you're adding this check of priv->domkey here because virt_viewer_lookup_domain() is not safe in the case that domkey is NULL? In that case, I think it would make more sense to add this check inside virt_viewer_lookup_domain() instead of adding it here. I think that it makes more sense to have the conditions for safe operation be checked by the function itself rather than the caller. > + ((dom = virt_viewer_lookup_domain(self)) == NULL && !priv->waitvm)) { > + dom = choose_vm_dialog(&priv->domkey, priv->conn, &err); > + if (dom == NULL) { > + if (err != NULL && In general, the convention is that functions with a GError** output parameter will always set the error when the return is NULL. So the (err != NULL) inside the (dom == NULL) is not strictly necessary. Also note that g_error_matches() can accept a NULL error. > + !g_error_matches(err, VIRT_VIEWER_ERROR, VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED)) { > + virt_viewer_app_simple_message_dialog(app, err->message); > + } > goto cleanup; > } > + } else if (!dom && priv->waitvm) { > + virt_viewer_app_show_status(app, _("Waiting for guest domain to be created")); > + virt_viewer_app_trace(app, "Guest %s does not yet exist, waiting for it to be created", > + priv->domkey); > + goto done; > } > > if (virDomainGetUUIDString(dom, uuid_string) < 0) { > @@ -579,14 +664,16 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > } else { > ret = virt_viewer_update_display(self, dom); > if (ret) > - ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, error); > + ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err); > if (!ret) { > if (priv->waitvm) { > virt_viewer_app_show_status(app, _("Waiting for guest domain to start server")); > virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting for it to start", > priv->domkey); > } else { > - g_debug("Failed to activate viewer"); > + g_set_error_literal(&err, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED, > + _("Failed to activate viewer")); > + g_debug(err->message); > goto cleanup; > } > } > @@ -595,6 +682,8 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error) > done: > ret = TRUE; > cleanup: > + if (err != NULL) > + g_propagate_error(error, err); > if (dom) > virDomainFree(dom); > return ret; > @@ -739,7 +828,8 @@ virt_viewer_connect(VirtViewerApp *app) > } > > if (!virt_viewer_app_initial_connect(app, &error)) { > - if (error) > + if (error && > + !g_error_matches(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED)) > g_warning("%s", error->message); > g_clear_error(&error); > return -1; _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
