[virt-tools-list] [PATCH virt-viewer] Connecting without specifying VM domain name
Jonathon Jongsma
jjongsma at redhat.com
Thu Oct 2 18:35:42 UTC 2014
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;
More information about the virt-tools-list
mailing list