[virt-tools-list] [PATCH] Add ability to define custom display->monitor mapping per vm
Jonathon Jongsma
jjongsma at redhat.com
Wed Oct 30 16:35:24 UTC 2013
Replies below
----- Original Message -----
> From: "Marc-André Lureau" <mlureau at redhat.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>
> Cc: virt-tools-list at redhat.com
> Sent: Wednesday, October 30, 2013 10:43:06 AM
> Subject: Re: [virt-tools-list] [PATCH] Add ability to define custom display->monitor mapping per vm
>
>
>
> ----- Original Message -----
> > Fullscreen mode generally just assigns display 1 to monitor 1, display 2 to
> > monitor 2, etc. For custom setups, you can define a monitor mapping in the
> > settings keyfile per-vm. This requires a vm uuid (so only works in
> > virt-viewer
> > or on versions of spice-server that send the uuid over the wire). The
> > format
> > is
> > pretty basic:
> >
> > [6485b20f-e9da-614c-72b0-60a7857e7886]
> > monitor-mapping=2;3
> >
> > The group name ("6485b20f-e9da-614c-72b0-60a7857e7886") is the uuid id of
> > the
> > vm. This group has a single key: monitor-mapping. This key is an array of
> > integers describing the order in which to assign the monitors to a guest
> > display. Any monitors that are not listed in this array will not be
> > configured
> > at startup. For instance:
> >
> > monitor-mapping=2;1
> >
> > will attempt to configure 2 displays on the guest and assign the first
> > display
> > to monitor 2 and the second display to monitor 1.
> >
> > monitor-mapping=2
> >
> > will only configure a single display on the guest and place it on the
> > second
> > monitor. Any monitor numbers listed in the keyfile are greater than the
> > number
> > of monitors that are physically present, they will be ignored.
>
> Looks good,
>
> > ---
> > src/virt-viewer-app.c | 101
> > ++++++++++++++++++++++++++++++++++++----
> > src/virt-viewer-app.h | 3 ++
> > src/virt-viewer-session-spice.c | 41 +++++++++++++---
> > src/virt-viewer-window.c | 9 +++-
> > src/virt-viewer.c | 7 +++
> > 5 files changed, 144 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index d071568..da25863 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -108,6 +108,7 @@ struct _VirtViewerAppPrivate {
> > VirtViewerWindow *main_window;
> > GtkWidget *main_notebook;
> > GHashTable *windows;
> > + GArray *initial_display_map;
> > gchar *clipboard;
> >
> > gboolean direct;
> > @@ -271,6 +272,82 @@ virt_viewer_app_quit(VirtViewerApp *self)
> > gtk_main_quit();
> > }
> >
> > +gint virt_viewer_app_get_n_initial_displays(VirtViewerApp* self)
> > +{
> > + if (self->priv->initial_display_map)
> > + return self->priv->initial_display_map->len;
> > +
> > + return gdk_screen_get_n_monitors(gdk_screen_get_default());
> > +}
> > +
> > +gint virt_viewer_app_get_initial_monitor_for_display(VirtViewerApp* self,
> > gint display)
> > +{
> > + gint monitor = -1;
> > +
> > + if (self->priv->initial_display_map) {
> > + if (display < self->priv->initial_display_map->len)
> > + monitor = g_array_index(self->priv->initial_display_map, gint,
> > display);
> > + } else {
> > + monitor = display;
> > + }
> > +
> > + return monitor;
> > +}
> > +
> > +
> > +void virt_viewer_app_set_uuid_string(VirtViewerApp*self, const gchar*
> > uuid_string)
> > +{
> > + GArray* mapping = NULL;
> > + GError* error = NULL;
> > + gsize ndisplays = 0;
> > + gint* displays = NULL;
> > + gint nmonitors = gdk_screen_get_n_monitors(gdk_screen_get_default());
> > +
> > + DEBUG_LOG("%s: UUID changed to %s", G_STRFUNC, uuid_string);
> > +
> > + displays = g_key_file_get_integer_list(self->priv->config,
> > + uuid_string, "monitor-mapping",
> > &ndisplays, &error);
> > + if (error) {
> > + if (error->code != G_KEY_FILE_ERROR_GROUP_NOT_FOUND)
> > + g_warning("Error reading monitor assignments: %s",
> > error->message);
> > + g_clear_error(&error);
> > + } else {
> > + int i = 0;
> > + mapping = g_array_sized_new(FALSE, FALSE, sizeof(displays[0]),
> > ndisplays);
> > + // config file format is 1-based, not 0-based
> > + for (i = 0; i < ndisplays; i++) {
> > + gint val = displays[i] - 1;
> > +
> > + // sanity check
> > + if (val >= nmonitors)
> > + g_warning("Initial monitor #%i for display #%i does not
> > exist, skipping...", val, i);
> > + else
> > + g_array_append_val(mapping, val);
> > + }
> > + g_free(displays);
> > + }
> > +
> > + if (self->priv->initial_display_map)
> > + g_array_unref(self->priv->initial_display_map);
> > +
> > + self->priv->initial_display_map = mapping;
> > +
> > + // if we're changing our initial display map, move any existing
> > windows
> > to
> > + // the appropriate monitors according to the per-vm configuration
> > + if (mapping && self->priv->fullscreen) {
> > + GHashTableIter iter;
> > + gpointer value;
> > + gint i = 0;
> > +
> > + g_hash_table_iter_init(&iter, self->priv->windows);
> > + while (g_hash_table_iter_next(&iter, NULL, &value)) {
> > + gint monitor =
> > virt_viewer_app_get_initial_monitor_for_display(self, i);
> > + virt_viewer_window_enter_fullscreen(VIRT_VIEWER_WINDOW(value),
> > monitor);
> > + i++;
> > + }
>
> You couldn't call app_window_try_fullscreen() ?
>
> It could be nice if the virt_viewer_app_get_initial_monitor_for_display()
> could be done there, in one place.
>
Hmm, I can't remember why I didn't use app_window_try_fullscreen() here. I seem to recall explicitly choosing _window_enter_fullscreen() rather than app_window_try_fullscreen(), but I don't see any reason why I can't use it now... However, I'm not sure that putting get_initial_monitor_for_display() into app_window_try_fullscreen() is a good idea, since that could theoretically be called after startup. And the get_initial_monitor...() functions are intended only for startup / window creation. In practice, perhaps this function never gets called after startup, so maybe it's ok to put it there?
> > + }
> > +}
> > +
> > void
> > virt_viewer_app_maybe_quit(VirtViewerApp *self, VirtViewerWindow *window)
> > {
> > @@ -674,7 +751,8 @@ virt_viewer_app_window_new(VirtViewerApp *self, gint
> > nth)
> > virt_viewer_window_set_zoom_level(window,
> > virt_viewer_window_get_zoom_level(self->priv->main_window));
> > virt_viewer_app_set_nth_window(self, nth, window);
> > if (self->priv->fullscreen)
> > - app_window_try_fullscreen(self, window, nth);
> > + app_window_try_fullscreen(self, window,
> > +
> > virt_viewer_app_get_initial_monitor_for_display(self,
> > nth));
> >
> > w = virt_viewer_window_get_window(window);
> > g_signal_connect(w, "hide", G_CALLBACK(viewer_window_visible_cb),
> > self);
> > @@ -745,6 +823,7 @@ virt_viewer_app_display_added(VirtViewerSession
> > *session
> > G_GNUC_UNUSED,
> > }
> >
> > window = virt_viewer_app_window_new(self, nth);
> > +
> > }
> > }
> >
> > @@ -1283,19 +1362,20 @@ virt_viewer_app_set_kiosk(VirtViewerApp *self,
> > gboolean enabled)
> > int i;
> >
> > self->priv->kiosk = enabled;
> > - if (enabled)
> > + if (enabled) {
>
> What was the reason for this change?
>
> We wanted to disable kiosk mode, now that won't update window kiosk state.
Ah. This was done to prevent creating N windows at startup if kiosk mode is false. But it's something that was done in an early part of the implementation of this patch, and it's not necessary now that I've added the "// if we're changing our initial display map, move any existing windows..." code in the section above. I'll remove this change from the patch (though I don't think unconditionally creating N windows at startup is necessarily a good idea).
>
> > virt_viewer_app_set_fullscreen(self, enabled);
> >
> > - for (i = 0; i < gdk_screen_get_n_monitors(gdk_screen_get_default());
> > i++) {
> > - VirtViewerWindow *win = virt_viewer_app_get_nth_window(self, i);
> > + for (i = 0; i <
> > gdk_screen_get_n_monitors(gdk_screen_get_default());
> > i++) {
> > + VirtViewerWindow *win = virt_viewer_app_get_nth_window(self,
> > i);
> >
> > - if (win == NULL)
> > - win = virt_viewer_app_window_new(self, i);
> > + if (win == NULL)
> > + win = virt_viewer_app_window_new(self, i);
> >
> > - if (enabled)
> > - virt_viewer_window_show(win);
> > + if (enabled)
> > + virt_viewer_window_show(win);
> >
> > - virt_viewer_window_set_kiosk(win, enabled);
> > + virt_viewer_window_set_kiosk(win, enabled);
> > + }
> > }
> > }
> >
> > @@ -1433,6 +1513,7 @@ virt_viewer_app_dispose (GObject *object)
> > g_free(priv->config_file);
> > priv->config_file = NULL;
> > g_clear_pointer(&priv->config, g_key_file_free);
> > + g_clear_pointer(&priv->initial_display_map, g_array_unref);
> >
> > virt_viewer_app_free_connect_info(self);
> >
> > @@ -1854,8 +1935,8 @@ static void fullscreen_cb(gpointer key,
> > gpointer value,
> > gpointer user_data)
> > {
> > - gint nth = *(gint*)key;
> > FullscreenOptions *options = (FullscreenOptions *)user_data;
> > + gint nth =
> > virt_viewer_app_get_initial_monitor_for_display(options->app,
> > *(gint*)key);
> > VirtViewerWindow *vwin = VIRT_VIEWER_WINDOW(value);
> >
> > DEBUG_LOG("fullscreen display %d: %d", nth, options->fullscreen);
> > diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h
> > index 4721fe3..6c9443a 100644
> > --- a/src/virt-viewer-app.h
> > +++ b/src/virt-viewer-app.h
> > @@ -99,6 +99,9 @@ VirtViewerSession*
> > virt_viewer_app_get_session(VirtViewerApp *self);
> > gboolean virt_viewer_app_get_fullscreen(VirtViewerApp *app);
> > gboolean virt_viewer_app_get_fullscreen_auto_conf(VirtViewerApp *app);
> > const GOptionEntry* virt_viewer_app_get_options(void);
> > +gint virt_viewer_app_get_n_initial_displays(VirtViewerApp* self);
> > +gint virt_viewer_app_get_initial_monitor_for_display(VirtViewerApp* self,
> > gint display);
> > +void virt_viewer_app_set_uuid_string(VirtViewerApp* self, const gchar*
> > uuid_string);
> >
> > G_END_DECLS
> >
> > diff --git a/src/virt-viewer-session-spice.c
> > b/src/virt-viewer-session-spice.c
> > index 4ed4aff..229d01e 100644
> > --- a/src/virt-viewer-session-spice.c
> > +++ b/src/virt-viewer-session-spice.c
> > @@ -28,6 +28,7 @@
> > #include <glib/gi18n.h>
> >
> > #include <spice-option.h>
> > +#include <spice-util.h>
> > #include <usb-device-widget.h>
> > #include "virt-viewer-file.h"
> > #include "virt-viewer-util.h"
> > @@ -51,6 +52,7 @@ struct _VirtViewerSessionSpicePrivate {
> > SpiceMainChannel *main_channel;
> > const SpiceAudio *audio;
> > int channel_count;
> > + int display_channel_count;
> > int usbredir_channel_count;
> > gboolean has_sw_smartcard_reader;
> > guint pass_try;
> > @@ -677,10 +679,11 @@ virt_viewer_session_spice_channel_new(SpiceSession
> > *s,
> > self->priv->main_channel = SPICE_MAIN_CHANNEL(channel);
> >
> > g_signal_connect(channel, "notify::agent-connected",
> > G_CALLBACK(agent_connected_changed), self);
> > - virt_viewer_session_spice_fullscreen_auto_conf(self);
> > }
> >
> > if (SPICE_IS_DISPLAY_CHANNEL(channel)) {
> > + self->priv->display_channel_count++;
> > + virt_viewer_session_spice_fullscreen_auto_conf(self);
> > g_signal_emit_by_name(session, "session-initialized");
> >
> > g_signal_connect(channel, "notify::monitors",
> > @@ -717,7 +720,8 @@
> > virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice
> > *self)
> > VirtViewerApp *app = NULL;
> > GdkRectangle dest;
> > gboolean agent_connected;
> > - gint i;
> > + gint i, j;
> > + gsize ndisplays = 0;
> >
> > app = virt_viewer_session_get_app(VIRT_VIEWER_SESSION(self));
> > g_return_val_if_fail(VIRT_VIEWER_IS_APP(app), TRUE);
> > @@ -730,21 +734,28 @@
> > virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice
> > *self)
> > DEBUG_LOG("no main channel yet");
> > return FALSE;
> > }
> > + if (self->priv->display_channel_count == 0) {
>
> Is this related? Why is it needed?
Yes, good question. It is related, but I admit that it's a bit of a hack. Let me explain what I'm trying to do here. Basically, in testing, I found that agent-connected is emitted quite early in the spice session. At the point in the session where the agent is connected, we still don't know the uuid or the name of the vm that we're connected to, because we haven't received those messages from the spice server yet. So we don't know how many displays we should configure or how to arrange them. From my testing, the CHANNELS_LIST message (which prompts the creation of the display channel and all of the other channels) comes after all of the preliminary uuid / etc messages are finished (if they are sent at all). So I use the existence of the display channel as a signal that we can start our display configuration. Perhaps it's not wise to rely on this sequence of events, however. So I'm open to other suggestions here.
>
> > + DEBUG_LOG("no display channel yet");
> > + return FALSE;
> > + }
> > g_object_get(cmain, "agent-connected", &agent_connected, NULL);
> > if (!agent_connected) {
> > DEBUG_LOG("Agent not connected, skipping autoconf");
> > return FALSE;
> > }
> >
> > - DEBUG_LOG("Performing full screen auto-conf, %d host monitors",
> > - gdk_screen_get_n_monitors(screen));
> > g_object_set(G_OBJECT(cmain),
> > "disable-display-position", FALSE,
> > "disable-display-align", TRUE,
> > NULL);
> > spice_main_set_display_enabled(cmain, -1, FALSE);
> > - for (i = 0; i < gdk_screen_get_n_monitors(screen); i++) {
> > - gdk_screen_get_monitor_geometry(screen, i, &dest);
> > +
> > + ndisplays = virt_viewer_app_get_n_initial_displays(app);
> > + DEBUG_LOG("Performing full screen auto-conf, %zd host monitors",
> > ndisplays);
> > +
> > + for (i = 0; i < ndisplays; i++) {
> > + j = virt_viewer_app_get_initial_monitor_for_display(app, i);
> > + gdk_screen_get_monitor_geometry(screen, j, &dest);
> > DEBUG_LOG("Set SPICE display %d to (%d,%d)-(%dx%d)",
> > i, dest.x, dest.y, dest.width, dest.height);
> > spice_main_set_display(cmain, i, dest.x, dest.y, dest.width,
> > dest.height);
> > @@ -777,6 +788,7 @@ virt_viewer_session_spice_channel_destroy(G_GNUC_UNUSED
> > SpiceSession *s,
> > if (SPICE_IS_DISPLAY_CHANNEL(channel)) {
> > VirtViewerDisplay *display = g_object_get_data(G_OBJECT(channel),
> > "virt-viewer-display");
> > DEBUG_LOG("zap display channel (#%d, %p)", id, display);
> > + self->priv->display_channel_count--;
> > }
> >
> > if (SPICE_IS_PLAYBACK_CHANNEL(channel) && self->priv->audio) {
> > @@ -804,6 +816,22 @@ fullscreen_changed(GObject *gobject G_GNUC_UNUSED,
> > virt_viewer_session_spice_fullscreen_auto_conf(self);
> > }
> >
> > +static void
> > +uuid_changed(GObject *gobject G_GNUC_UNUSED,
> > + GParamSpec *pspec G_GNUC_UNUSED,
> > + VirtViewerSessionSpice *self)
> > +{
> > + guint8* uuid = NULL;
> > + gchar* uuid_str = NULL;
> > + VirtViewerApp* app =
> > virt_viewer_session_get_app(VIRT_VIEWER_SESSION(self));
> > +
> > + g_object_get(self->priv->session, "uuid", &uuid, NULL);
> > + uuid_str = spice_uuid_to_string(uuid);
> > + virt_viewer_app_set_uuid_string(app, uuid_str);
> > + g_free(uuid_str);
> > +
> > +}
> > +
> > VirtViewerSession *
> > virt_viewer_session_spice_new(VirtViewerApp *app, GtkWindow *main_window)
> > {
> > @@ -815,6 +843,7 @@ virt_viewer_session_spice_new(VirtViewerApp *app,
> > GtkWindow *main_window)
> > self->priv->main_window = g_object_ref(main_window);
> >
> > g_signal_connect(app, "notify::fullscreen",
> > G_CALLBACK(fullscreen_changed), self);
> > + g_signal_connect(self->priv->session, "notify::uuid",
> > G_CALLBACK(uuid_changed), self);
> >
> > return VIRT_VIEWER_SESSION(self);
> > }
> > diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
> > index 5ce1d98..ac2ec3e 100644
> > --- a/src/virt-viewer-window.c
> > +++ b/src/virt-viewer-window.c
> > @@ -496,7 +496,9 @@ virt_viewer_window_leave_fullscreen(VirtViewerWindow
> > *self)
> > if (!priv->fullscreen)
> > return;
> >
> > + g_signal_handlers_block_by_func(check,
> > virt_viewer_window_menu_view_fullscreen, self);
> > gtk_check_menu_item_set_active(check, FALSE);
> > + g_signal_handlers_unblock_by_func(check,
> > virt_viewer_window_menu_view_fullscreen, self);
> > priv->fullscreen = FALSE;
> > priv->fullscreen_monitor = -1;
> > if (priv->display) {
> > @@ -528,18 +530,23 @@ virt_viewer_window_enter_fullscreen(VirtViewerWindow
> > *self, gint monitor)
> > GtkWidget *menu = GTK_WIDGET(gtk_builder_get_object(priv->builder,
> > "top-menu"));
> > GtkCheckMenuItem *check =
> > GTK_CHECK_MENU_ITEM(gtk_builder_get_object(priv->builder,
> > "menu-view-fullscreen"));
> >
> > + if (priv->fullscreen && priv->fullscreen_monitor != monitor)
> > + virt_viewer_window_leave_fullscreen(self);
> > +
> > if (priv->fullscreen)
> > return;
> >
> > - priv->fullscreen_monitor = monitor;
> > priv->fullscreen = TRUE;
> > + priv->fullscreen_monitor = monitor;
> >
> > if (!gtk_widget_get_mapped(priv->window)) {
> > g_signal_connect(priv->window, "map-event", G_CALLBACK(mapped),
> > self);
> > return;
> > }
> >
> > + g_signal_handlers_block_by_func(check,
> > virt_viewer_window_menu_view_fullscreen, self);
> > gtk_check_menu_item_set_active(check, TRUE);
> > + g_signal_handlers_unblock_by_func(check,
> > virt_viewer_window_menu_view_fullscreen, self);
> > gtk_widget_hide(menu);
> > gtk_widget_show(priv->toolbar);
> > ViewAutoDrawer_SetActive(VIEW_AUTODRAWER(priv->layout), TRUE);
> > diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> > index ae25fc6..e1553fd 100644
> > --- a/src/virt-viewer.c
> > +++ b/src/virt-viewer.c
> > @@ -530,6 +530,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError
> > **error)
> > gboolean ret = FALSE;
> > VirtViewer *self = VIRT_VIEWER(app);
> > VirtViewerPrivate *priv = self->priv;
> > + char uuid_string[VIR_UUID_STRING_BUFLEN];
> >
> > DEBUG_LOG("initial connect");
> >
> > @@ -555,6 +556,12 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError
> > **error)
> > }
> > }
> >
> > + if (virDomainGetUUIDString(dom, uuid_string) < 0) {
> > + DEBUG_LOG("Couldn't get uuid from libvirt");
> > + } else {
> > + virt_viewer_app_set_uuid_string(app, uuid_string);
> > + }
> > +
> > virt_viewer_app_show_status(app, _("Checking guest domain status"));
> > if (virDomainGetInfo(dom, &info) < 0) {
> > DEBUG_LOG("Cannot get guest state");
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > 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