[PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[sweeaun]

 -display gtk,monitor=<value>

Signed-off-by: sweeaun <swee.aun.khor@intel.com>
---
 qapi/ui.json    |  4 +++-
 qemu-options.hx |  2 +-
 ui/gtk.c        | 15 +++++++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 1052ca9c38..1616f3ffbd 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1035,13 +1035,15 @@
 #               assuming the guest will resize the display to match
 #               the window size then.  Otherwise it defaults to "off".
 #               Since 3.1
+# @monitor: Monitor number to display qemu in full screen.
 #
 # Since: 2.12
 #
 ##
 { 'struct'  : 'DisplayGTK',
   'data'    : { '*grab-on-hover' : 'bool',
-                '*zoom-to-fit'   : 'bool'  } }
+                '*zoom-to-fit'   : 'bool',
+                '*monitor' : 'int' } }
 
 ##
 # @DisplayEGLHeadless:
diff --git a/qemu-options.hx b/qemu-options.hx
index 14258784b3..e4b89b6a72 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1787,7 +1787,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
     "            [,window_close=on|off][,gl=on|core|es|off]\n"
 #endif
 #if defined(CONFIG_GTK)
-    "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
+    "-display gtk[,grab_on_hover=on|off][,gl=on|off][,monitor=<value>]\n"
 #endif
 #if defined(CONFIG_VNC)
     "-display vnc=<display>[,<optargs>]\n"
diff --git a/ui/gtk.c b/ui/gtk.c
index 98046f577b..84da126611 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2268,6 +2268,21 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
         gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
     }
     gd_clipboard_init(s);
+
+    if (opts->u.gtk.has_monitor) {
+        int n_monitor;
+        n_monitor = gdk_display_get_n_monitors(window_display);
+
+        if ((opts->u.gtk.monitor <= n_monitor) &&
+            (opts->u.gtk.monitor > 0)) {
+            GdkScreen *gdk_screen;
+            gdk_screen = gdk_display_get_default_screen(window_display);
+            gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
+                                             (opts->u.gtk.monitor - 1));
+        } else {
+            fprintf(stderr, "Invalid GTK monitor argument\n");
+        }
+    }
 }
 
 static void early_gtk_display_init(DisplayOptions *opts)
-- 
2.24.3

Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Markus Armbruster]

You neglected to cc: the Graphics maintainer.  I'm doing that for you
now.

sweeaun <swee.aun.khor@intel.com> writes:

>  -display gtk,monitor=<value>
>
> Signed-off-by: sweeaun <swee.aun.khor@intel.com>

Your commit message is formatted badly.  What about this:

    ui/gtk: New -display gtk parameter 'monitor'.

    This lets the user select monitor number to display QEMU in full
    screen with -display gtk,monitor=<value>.

Furthermore, you're Signed-off-by line may be off.  It should be of the
form

    Signed-off-by: REAL NAME <EMAIL>

Is "sweeaun" your real name?

> ---
>  qapi/ui.json    |  4 +++-
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 15 +++++++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 1052ca9c38..1616f3ffbd 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1035,13 +1035,15 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> +# @monitor: Monitor number to display qemu in full screen.

We spell it QEMU.

Should "full screen" be "full-screen" or even "fullscreen"?

>  #
>  # Since: 2.12
>  #
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*monitor' : 'int' } }

Best to make your addition "blend in" like this

   { 'struct'  : 'DisplayGTK',
     'data'    : { '*grab-on-hover' : 'bool',
                   '*zoom-to-fit'   : 'bool',
                   '*monitor'       : 'int' } }

>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 14258784b3..e4b89b6a72 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1787,7 +1787,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>      "            [,window_close=on|off][,gl=on|core|es|off]\n"
>  #endif
>  #if defined(CONFIG_GTK)
> -    "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
> +    "-display gtk[,grab_on_hover=on|off][,gl=on|off][,monitor=<value>]\n"
>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 98046f577b..84da126611 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2268,6 +2268,21 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
>      }
>      gd_clipboard_init(s);
> +
> +    if (opts->u.gtk.has_monitor) {
> +        int n_monitor;
> +        n_monitor = gdk_display_get_n_monitors(window_display);

Terser:

           int n_monitor = gdk_display_get_n_monitors(window_display);

> +
> +        if ((opts->u.gtk.monitor <= n_monitor) &&
> +            (opts->u.gtk.monitor > 0)) {

Suggest to drop the superfluous parenthesis:

           if (opts->u.gtk.monitor <= n_monitor &&
               opts->u.gtk.monitor > 0) {

> +            GdkScreen *gdk_screen;
> +            gdk_screen = gdk_display_get_default_screen(window_display);
> +            gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
> +                                             (opts->u.gtk.monitor - 1));

Drop the superfluous parenthesis around the last argument.

Your new option argument seems to count monitors from 1, while GTK
counts them from zero.  Why the difference?

Your documentation states that @monitor applies only "in full screen",
but this code is not guarded by if (opts->full_screen).  Why is that
okay?

From gdk_display_get_n_monitors()'s documentation: "The returned number
is valid until the next emission of the “monitor-added” or
“monitor-removed” signal."  This suggests monitors can come and go at
any time.  If they can, what happens when the monitor we're trying to
use here has gone away since we called gdk_display_get_n_monitors()?

> +        } else {
> +            fprintf(stderr, "Invalid GTK monitor argument\n");
> +        }
> +    }
>  }
>  
>  static void early_gtk_display_init(DisplayOptions *opts)

RE: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Khor, Swee Aun]

Hi Markus,
Thanks for include Graphic maintainer and the coding style comments.  Yes, sweeaun is my name 😊

For  "full screen" be "full-screen" or even "fullscreen"? These 3 words have been being used in QEMU repo, but full-screen mostly used for variable/member.
Thus, I felt full screen should be fine to be used as documentation. What do you think?
Example:
"FullScreen" in hw/arm/nseries.c
"toggle full screen" in softmmu/vl.c 
""full-screen" in /qemu-options.hx

For subsequence questions:
" Your new option argument seems to count monitors from 1, while GTK counts them from zero.  Why the difference?"
sweeaun: It is due to gtk_window_fullscreen_on_monitor monitor index is started from zero. I am not using zero as starting index of new option argument to make easier for user. Example, if there is 2 monitors, then argument option can be monitor 1 or 2. Last number will matched with total monitors which can avoid confusion for user. That is my thought.   

"Your documentation states that @monitor applies only "in full screen", but this code is not guarded by if (opts->full_screen).  Why is that okay?"
sweeaun: It doesn’t need to be guarded by if (opts->full_screen), as gtk_window_fullscreen_on_monitor will enable the full screen by itself.  

"From gdk_display_get_n_monitors()'s documentation: "The returned number is valid until the next emission of the “monitor-added” or “monitor-removed” signal."  This suggests monitors can come and go at any time.  If they can, what happens when the monitor we're trying to use here has gone away since we called gdk_display_get_n_monitors()?"
sweeaun: Based on my observation, when specific monitor device disconnected after QEMU launched on it, QEMU application will not be visible but QEMU application still running and screen framebuffer size is not being changed at all. QEMU application will be visible once you connect back the monitor. 
Yes the return number from gdk_display_get_n_monitors() will minus 1 compared to previous once 1 monitor has been disconnected. 

Regards,
SweeAun

-----Original Message-----
From: Markus Armbruster <armbru@redhat.com> 
Sent: Friday, June 18, 2021 7:07 PM
To: Khor, Swee Aun <swee.aun.khor@intel.com>
Cc: qemu-devel@nongnu.org; Romli, Khairul Anuar <khairul.anuar.romli@intel.com>; eblake@redhat.com; Kasireddy, Vivek <vivek.kasireddy@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

You neglected to cc: the Graphics maintainer.  I'm doing that for you now.

sweeaun <swee.aun.khor@intel.com> writes:

>  -display gtk,monitor=<value>
>
> Signed-off-by: sweeaun <swee.aun.khor@intel.com>

Your commit message is formatted badly.  What about this:

    ui/gtk: New -display gtk parameter 'monitor'.

    This lets the user select monitor number to display QEMU in full
    screen with -display gtk,monitor=<value>.

Furthermore, you're Signed-off-by line may be off.  It should be of the form

    Signed-off-by: REAL NAME <EMAIL>

Is "sweeaun" your real name?

> ---
>  qapi/ui.json    |  4 +++-
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 15 +++++++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json index 1052ca9c38..1616f3ffbd 
> 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1035,13 +1035,15 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> +# @monitor: Monitor number to display qemu in full screen.

We spell it QEMU.

Should "full screen" be "full-screen" or even "fullscreen"?

>  #
>  # Since: 2.12
>  #
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*monitor' : 'int' } }

Best to make your addition "blend in" like this

   { 'struct'  : 'DisplayGTK',
     'data'    : { '*grab-on-hover' : 'bool',
                   '*zoom-to-fit'   : 'bool',
                   '*monitor'       : 'int' } }

>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx index 
> 14258784b3..e4b89b6a72 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1787,7 +1787,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>      "            [,window_close=on|off][,gl=on|core|es|off]\n"
>  #endif
>  #if defined(CONFIG_GTK)
> -    "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
> +    "-display gtk[,grab_on_hover=on|off][,gl=on|off][,monitor=<value>]\n"
>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 98046f577b..84da126611 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2268,6 +2268,21 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
>      }
>      gd_clipboard_init(s);
> +
> +    if (opts->u.gtk.has_monitor) {
> +        int n_monitor;
> +        n_monitor = gdk_display_get_n_monitors(window_display);

Terser:

           int n_monitor = gdk_display_get_n_monitors(window_display);

> +
> +        if ((opts->u.gtk.monitor <= n_monitor) &&
> +            (opts->u.gtk.monitor > 0)) {

Suggest to drop the superfluous parenthesis:

           if (opts->u.gtk.monitor <= n_monitor &&
               opts->u.gtk.monitor > 0) {

> +            GdkScreen *gdk_screen;
> +            gdk_screen = gdk_display_get_default_screen(window_display);
> +            gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
> +                                             (opts->u.gtk.monitor - 
> + 1));

Drop the superfluous parenthesis around the last argument.

Your new option argument seems to count monitors from 1, while GTK counts them from zero.  Why the difference?

Your documentation states that @monitor applies only "in full screen", but this code is not guarded by if (opts->full_screen).  Why is that okay?

From gdk_display_get_n_monitors()'s documentation: "The returned number is valid until the next emission of the “monitor-added” or “monitor-removed” signal."  This suggests monitors can come and go at any time.  If they can, what happens when the monitor we're trying to use here has gone away since we called gdk_display_get_n_monitors()?

> +        } else {
> +            fprintf(stderr, "Invalid GTK monitor argument\n");
> +        }
> +    }
>  }
>  
>  static void early_gtk_display_init(DisplayOptions *opts)

Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Gerd Hoffmann]

  Hi,

> " Your new option argument seems to count monitors from 1, while GTK counts them from zero.  Why the difference?"
> sweeaun: It is due to gtk_window_fullscreen_on_monitor monitor index is started from zero. I am not using zero as starting index of new option argument to make easier for user. Example, if there is 2 monitors, then argument option can be monitor 1 or 2. Last number will matched with total monitors which can avoid confusion for user. That is my thought.   

Start counting from zero makes sense too.  Matter of tasts.  We have
examples for both in the qemu source tree.  So, whatever you pick, this
clearly needs documentation (in ui.json option description).

> "Your documentation states that @monitor applies only "in full screen", but this code is not guarded by if (opts->full_screen).  Why is that okay?"
> sweeaun: It doesn’t need to be guarded by if (opts->full_screen), as gtk_window_fullscreen_on_monitor will enable the full screen by itself.  

Well, wouldn't it make sense to have monitor=<nr> work for both
full-screen=on and full-screen=off cases?

> sweeaun: Based on my observation, when specific monitor device disconnected after QEMU launched on it, QEMU application will not be visible but QEMU application still running and screen framebuffer size is not being changed at all. QEMU application will be visible once you connect back the monitor. 

Well, that probably depends on the display server and might even be
configurable.  I've seen different ways to handle that, most common ones
being (a) do nothing or (b) trying move all windows to the monitor which
is still connected.

I don't think qemu has to worry much here, and trying to automatically
adapt to hot-plugged monitors might even have bad interactions with
whatever the display server is going to do.

take care,
  Gerd

RE: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Khor, Swee Aun]

Hi Gerd,

Start counting from zero makes sense too.  Matter of tasts.  We have examples for both in the qemu source tree.  So, whatever you pick, this clearly needs documentation (in ui.json option description).
sweeaun: Sure, I will make sure that is mentioned clearly in documentation.  

Well, wouldn't it make sense to have monitor=<nr> work for both full-screen=on and full-screen=off cases?
sweeaun:  Yes. That will be better option for user. However, I not managed to find other GTK window API that can set window into monitor rather than gtk_window_fullscreen_on_monitor so far.    

Well, that probably depends on the display server and might even be configurable.  I've seen different ways to handle that, most common ones being (a) do nothing or (b) trying move all windows to the monitor which is still connected.
I don't think qemu has to worry much here, and trying to automatically adapt to hot-plugged monitors might even have bad interactions with whatever the display server is going to do.
sweeaun: Alright. 

Regards,
SweeAun

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Monday, June 21, 2021 2:52 PM
To: Khor, Swee Aun <swee.aun.khor@intel.com>
Cc: Markus Armbruster <armbru@redhat.com>; Romli, Khairul Anuar <khairul.anuar.romli@intel.com>; eblake@redhat.com; qemu-devel@nongnu.org; Kasireddy, Vivek <vivek.kasireddy@intel.com>
Subject: Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

  Hi,

> " Your new option argument seems to count monitors from 1, while GTK counts them from zero.  Why the difference?"
> sweeaun: It is due to gtk_window_fullscreen_on_monitor monitor index is started from zero. I am not using zero as starting index of new option argument to make easier for user. Example, if there is 2 monitors, then argument option can be monitor 1 or 2. Last number will matched with total monitors which can avoid confusion for user. That is my thought.   

Start counting from zero makes sense too.  Matter of tasts.  We have examples for both in the qemu source tree.  So, whatever you pick, this clearly needs documentation (in ui.json option description).

> "Your documentation states that @monitor applies only "in full screen", but this code is not guarded by if (opts->full_screen).  Why is that okay?"
> sweeaun: It doesn’t need to be guarded by if (opts->full_screen), as gtk_window_fullscreen_on_monitor will enable the full screen by itself.  

Well, wouldn't it make sense to have monitor=<nr> work for both full-screen=on and full-screen=off cases?

> sweeaun: Based on my observation, when specific monitor device disconnected after QEMU launched on it, QEMU application will not be visible but QEMU application still running and screen framebuffer size is not being changed at all. QEMU application will be visible once you connect back the monitor. 

Well, that probably depends on the display server and might even be configurable.  I've seen different ways to handle that, most common ones being (a) do nothing or (b) trying move all windows to the monitor which is still connected.

I don't think qemu has to worry much here, and trying to automatically adapt to hot-plugged monitors might even have bad interactions with whatever the display server is going to do.

take care,
  Gerd

Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Gerd Hoffmann]

  Hi,

> Well, wouldn't it make sense to have monitor=<nr> work for both full-screen=on and full-screen=off cases?
> sweeaun:  Yes. That will be better option for user. However, I not managed to find other GTK window API that can set window into monitor rather than gtk_window_fullscreen_on_monitor so far.    

Hmm, right, nothing obvious at https://developer.gnome.org/gtk3/stable/GtkWindow.html for the non-full-screen case.

Nevertheless the options should have sane semantics.  We could:

  (1) Require full-screen=on in addition to monitor=<nr>.
      That would leave the door open to implement the full-screen=off
      case later if we figure some way to do that.
  (2) Rename the option to full-screen-on-monitor, to make clear this
      automatically enables fullscreen too.

take care,
  Gerd

RE: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Khor, Swee Aun]

Hi Gerg and Markus,

Hmm, right, nothing obvious at https://developer.gnome.org/gtk3/stable/GtkWindow.html for the non-full-screen case.

Nevertheless the options should have sane semantics.  We could:

  (1) Require full-screen=on in addition to monitor=<nr>.
      That would leave the door open to implement the full-screen=off
      case later if we figure some way to do that.
  (2) Rename the option to full-screen-on-monitor, to make clear this
      automatically enables fullscreen too.

sweeaun:  Alright, I would opt for option 2 in this case. Thanks for the feedback from you and Markus , I shall rework all the feedbacks for the v3.   

Regards,
SweeAun

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com> 
Sent: Monday, June 21, 2021 7:15 PM
To: Khor, Swee Aun <swee.aun.khor@intel.com>
Cc: Romli, Khairul Anuar <khairul.anuar.romli@intel.com>; Kasireddy, Vivek <vivek.kasireddy@intel.com>; eblake@redhat.com; Markus Armbruster <armbru@redhat.com>; qemu-devel@nongnu.org
Subject: Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

  Hi,

> Well, wouldn't it make sense to have monitor=<nr> work for both full-screen=on and full-screen=off cases?
> sweeaun:  Yes. That will be better option for user. However, I not managed to find other GTK window API that can set window into monitor rather than gtk_window_fullscreen_on_monitor so far.    

Hmm, right, nothing obvious at https://developer.gnome.org/gtk3/stable/GtkWindow.html for the non-full-screen case.

Nevertheless the options should have sane semantics.  We could:

  (1) Require full-screen=on in addition to monitor=<nr>.
      That would leave the door open to implement the full-screen=off
      case later if we figure some way to do that.
  (2) Rename the option to full-screen-on-monitor, to make clear this
      automatically enables fullscreen too.

take care,
  Gerd

Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:


[...]

>> sweeaun: Based on my observation, when specific monitor device disconnected after QEMU launched on it, QEMU application will not be visible but QEMU application still running and screen framebuffer size is not being changed at all. QEMU application will be visible once you connect back the monitor. 
>
> Well, that probably depends on the display server and might even be
> configurable.  I've seen different ways to handle that, most common ones
> being (a) do nothing or (b) trying move all windows to the monitor which
> is still connected.
>
> I don't think qemu has to worry much here, and trying to automatically
> adapt to hot-plugged monitors might even have bad interactions with
> whatever the display server is going to do.

I'm concerned there is a TOCTTOU issue:

    if (opts->u.gtk.has_monitor) {
        int n_monitor;
1.      n_monitor = gdk_display_get_n_monitors(window_display);

2.      if ((opts->u.gtk.monitor <= n_monitor) &&
            (opts->u.gtk.monitor > 0)) {
            GdkScreen *gdk_screen;
            gdk_screen = gdk_display_get_default_screen(window_display);
3.          gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
                                             (opts->u.gtk.monitor - 1));
        } else {
            fprintf(stderr, "Invalid GTK monitor argument\n");
        }
    }

If monitors can go at any time, then the check 2. cannot ensure we pass
a valid monitor number at 3.

I asked what happens when we pass an invalid monitor number.  I'm not
sure I understand sweeaun's answer.

If what happens is sane, then why have check 2.?

RE: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Khor, Swee Aun]

>> sweeaun: Based on my observation, when specific monitor device disconnected after QEMU launched on it, QEMU application will not be visible but QEMU application still running and screen framebuffer size is not being changed at all. QEMU application will be visible once you connect back the monitor. 
>
> Well, that probably depends on the display server and might even be 
> configurable.  I've seen different ways to handle that, most common 
> ones being (a) do nothing or (b) trying move all windows to the 
> monitor which is still connected.
>
> I don't think qemu has to worry much here, and trying to automatically 
> adapt to hot-plugged monitors might even have bad interactions with 
> whatever the display server is going to do.

I'm concerned there is a TOCTTOU issue:

    if (opts->u.gtk.has_monitor) {
        int n_monitor;
1.      n_monitor = gdk_display_get_n_monitors(window_display);

2.      if ((opts->u.gtk.monitor <= n_monitor) &&
            (opts->u.gtk.monitor > 0)) {
            GdkScreen *gdk_screen;
            gdk_screen = gdk_display_get_default_screen(window_display);
3.          gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
                                             (opts->u.gtk.monitor - 1));
        } else {
            fprintf(stderr, "Invalid GTK monitor argument\n");
        }
    }

If monitors can go at any time, then the check 2. cannot ensure we pass a valid monitor number at 3.

I asked what happens when we pass an invalid monitor number.  I'm not sure I understand sweeaun's answer.

If what happens is sane, then why have check 2.?

sweeaun: Sorry, I misunderstood your question, I thought your question is about what happened if monitor disconnected after the QEMU launched.
You are right, monitor could be disconnected in between 1 and 3. If invalid monitor passed into gtk_window_fullscreen_on_monitor then QEMU window will just be launched as normal without full screen and not the monitor that user specify.  I should check whether full screen has been successful taken in place since gtk_window_fullscreen_on_monitor function always return void.  

Regards,
SweeAun

-----Original Message-----
From: Markus Armbruster <armbru@redhat.com> 
Sent: Monday, June 21, 2021 4:31 PM
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: Khor, Swee Aun <swee.aun.khor@intel.com>; Romli, Khairul Anuar <khairul.anuar.romli@intel.com>; Kasireddy, Vivek <vivek.kasireddy@intel.com>; eblake@redhat.com; qemu-devel@nongnu.org
Subject: Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

Gerd Hoffmann <kraxel@redhat.com> writes:


[...]

>> sweeaun: Based on my observation, when specific monitor device disconnected after QEMU launched on it, QEMU application will not be visible but QEMU application still running and screen framebuffer size is not being changed at all. QEMU application will be visible once you connect back the monitor. 
>
> Well, that probably depends on the display server and might even be 
> configurable.  I've seen different ways to handle that, most common 
> ones being (a) do nothing or (b) trying move all windows to the 
> monitor which is still connected.
>
> I don't think qemu has to worry much here, and trying to automatically 
> adapt to hot-plugged monitors might even have bad interactions with 
> whatever the display server is going to do.

I'm concerned there is a TOCTTOU issue:

    if (opts->u.gtk.has_monitor) {
        int n_monitor;
1.      n_monitor = gdk_display_get_n_monitors(window_display);

2.      if ((opts->u.gtk.monitor <= n_monitor) &&
            (opts->u.gtk.monitor > 0)) {
            GdkScreen *gdk_screen;
            gdk_screen = gdk_display_get_default_screen(window_display);
3.          gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
                                             (opts->u.gtk.monitor - 1));
        } else {
            fprintf(stderr, "Invalid GTK monitor argument\n");
        }
    }

If monitors can go at any time, then the check 2. cannot ensure we pass a valid monitor number at 3.

I asked what happens when we pass an invalid monitor number.  I'm not sure I understand sweeaun's answer.

If what happens is sane, then why have check 2.?

Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Gerd Hoffmann]

> > I don't think qemu has to worry much here, and trying to automatically
> > adapt to hot-plugged monitors might even have bad interactions with
> > whatever the display server is going to do.
> 
> I'm concerned there is a TOCTTOU issue:
> 
>     if (opts->u.gtk.has_monitor) {
>         int n_monitor;
> 1.      n_monitor = gdk_display_get_n_monitors(window_display);
> 
> 2.      if ((opts->u.gtk.monitor <= n_monitor) &&
>             (opts->u.gtk.monitor > 0)) {
>             GdkScreen *gdk_screen;
>             gdk_screen = gdk_display_get_default_screen(window_display);
> 3.          gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), gdk_screen,
>                                              (opts->u.gtk.monitor - 1));
>         } else {
>             fprintf(stderr, "Invalid GTK monitor argument\n");
>         }
>     }
> 
> If monitors can go at any time, then the check 2. cannot ensure we pass
> a valid monitor number at 3.
> 
> I asked what happens when we pass an invalid monitor number.  I'm not
> sure I understand sweeaun's answer.
> 
> If what happens is sane, then why have check 2.?

gtk_window_fullscreen_on_monitor() seems to be a "best effort" offer and
it doesn't return error codes.  So catching possible user errors looks
useful to me.  The code should use error_report instead though (or maybe
warn_report given this is not fatal).  Reporting the valid range is
probably a good idea too.

take care,
  Gerd

Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Markus Armbruster]

"Khor, Swee Aun" <swee.aun.khor@intel.com> writes:

> Hi Markus,
> Thanks for include Graphic maintainer and the coding style comments.  Yes, sweeaun is my name 😊

I'd expect something like

    Signed-off-by: Khor, Swee Aun <swee.aun.khor@intel.com>