[PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Claudio Fontana]

The GTK Clipboard implementation may cause guest hangs.

Therefore implement a new configure switch --enable-gtk-clipboard,
disabled by default, as a meson option.

Regenerate the meson build options to include it.

The initialization of the clipboard is gtk.c, as well as the
compilation of gtk-clipboard.c are now conditional on this new option
to be set.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 meson.build                   | 9 +++++++++
 meson_options.txt             | 7 +++++++
 scripts/meson-buildoptions.sh | 3 +++
 ui/gtk.c                      | 2 ++
 ui/meson.build                | 5 ++++-
 5 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 1d448272ab..8bb96e5e8c 100644
--- a/meson.build
+++ b/meson.build
@@ -1243,6 +1243,8 @@ if nettle.found() and gmp.found()
 endif
 
 
+have_gtk_clipboard = false
+
 gtk = not_found
 gtkx11 = not_found
 vte = not_found
@@ -1258,12 +1260,18 @@ if not get_option('gtk').auto() or have_system
                         kwargs: static_kwargs)
     gtk = declare_dependency(dependencies: [gtk, gtkx11])
 
+    have_gtk_clipboard = get_option('gtk_clipboard').enabled()
+
     if not get_option('vte').auto() or have_system
       vte = dependency('vte-2.91',
                        method: 'pkg-config',
                        required: get_option('vte'),
                        kwargs: static_kwargs)
     endif
+  else
+    if get_option('gtk_clipboard').enabled()
+      error('GTK clipboard requested, but GTK not found')
+    endif
   endif
 endif
 
@@ -1842,6 +1850,7 @@ if glusterfs.found()
 endif
 config_host_data.set('CONFIG_GTK', gtk.found())
 config_host_data.set('CONFIG_VTE', vte.found())
+config_host_data.set('CONFIG_GTK_CLIPBOARD', have_gtk_clipboard)
 config_host_data.set('CONFIG_LIBATTR', have_old_libattr)
 config_host_data.set('CONFIG_LIBCAP_NG', libcap_ng.found())
 config_host_data.set('CONFIG_EBPF', libbpf.found())
diff --git a/meson_options.txt b/meson_options.txt
index 66128178bf..4b749ca549 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -219,6 +219,13 @@ option('vnc_sasl', type : 'feature', value : 'auto',
        description: 'SASL authentication for VNC server')
 option('vte', type : 'feature', value : 'auto',
        description: 'vte support for the gtk UI')
+
+# GTK Clipboard implementation is disabled by default, since it may cause hangs
+# of the guest VCPUs. See gitlab issue 1150:
+# https://gitlab.com/qemu-project/qemu/-/issues/1150
+
+option('gtk_clipboard', type: 'feature', value : 'disabled',
+       description: 'clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)')
 option('xkbcommon', type : 'feature', value : 'auto',
        description: 'xkbcommon support')
 option('zstd', type : 'feature', value : 'auto',
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 2cb0de5601..a542853bfd 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -93,6 +93,7 @@ meson_options_help() {
   printf "%s\n" '  glusterfs       Glusterfs block device driver'
   printf "%s\n" '  gnutls          GNUTLS cryptography support'
   printf "%s\n" '  gtk             GTK+ user interface'
+  printf "%s\n" '  gtk-clipboard   clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)'
   printf "%s\n" '  guest-agent     Build QEMU Guest Agent'
   printf "%s\n" '  guest-agent-msi Build MSI package for the QEMU Guest Agent'
   printf "%s\n" '  hax             HAX acceleration support'
@@ -274,6 +275,8 @@ _meson_option_parse() {
     --disable-gprof) printf "%s" -Dgprof=false ;;
     --enable-gtk) printf "%s" -Dgtk=enabled ;;
     --disable-gtk) printf "%s" -Dgtk=disabled ;;
+    --enable-gtk-clipboard) printf "%s" -Dgtk_clipboard=enabled ;;
+    --disable-gtk-clipboard) printf "%s" -Dgtk_clipboard=disabled ;;
     --enable-guest-agent) printf "%s" -Dguest_agent=enabled ;;
     --disable-guest-agent) printf "%s" -Dguest_agent=disabled ;;
     --enable-guest-agent-msi) printf "%s" -Dguest_agent_msi=enabled ;;
diff --git a/ui/gtk.c b/ui/gtk.c
index 7ec21f7798..4817623c8f 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2403,7 +2403,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
         opts->u.gtk.show_tabs) {
         gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
     }
+#ifdef CONFIG_GTK_CLIPBOARD
     gd_clipboard_init(s);
+#endif /* CONFIG_GTK_CLIPBOARD */
 }
 
 static void early_gtk_display_init(DisplayOptions *opts)
diff --git a/ui/meson.build b/ui/meson.build
index ec13949776..c1b137bf33 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -97,7 +97,10 @@ if gtk.found()
   softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('win32-kbd-hook.c'))
 
   gtk_ss = ss.source_set()
-  gtk_ss.add(gtk, vte, pixman, files('gtk.c', 'gtk-clipboard.c'))
+  gtk_ss.add(gtk, vte, pixman, files('gtk.c'))
+  if have_gtk_clipboard
+    gtk_ss.add(files('gtk-clipboard.c'))
+  endif
   gtk_ss.add(when: x11, if_true: files('x_keymap.c'))
   gtk_ss.add(when: opengl, if_true: files('gtk-gl-area.c'))
   gtk_ss.add(when: [x11, opengl], if_true: files('gtk-egl.c'))
-- 
2.26.2

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Gerd Hoffmann]

On Tue, Nov 08, 2022 at 05:23:24PM +0100, Claudio Fontana wrote:
> The GTK Clipboard implementation may cause guest hangs.
> 
> Therefore implement a new configure switch --enable-gtk-clipboard,
> disabled by default, as a meson option.

Hmm, I was thinking about a runtime option, add 'clipboard' bool to
DisplayGTK in qapi/ui.json) and just skip the call to
gd_clipboard_init() unless the option is explicitly enabled ...

I don't feel like vetoing a compile time option though, so in case you
prefer to stick with this:

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Claudio Fontana]

On 11/9/22 09:04, Gerd Hoffmann wrote:
> On Tue, Nov 08, 2022 at 05:23:24PM +0100, Claudio Fontana wrote:
>> The GTK Clipboard implementation may cause guest hangs.
>>
>> Therefore implement a new configure switch --enable-gtk-clipboard,
>> disabled by default, as a meson option.
> 
> Hmm, I was thinking about a runtime option, add 'clipboard' bool to
> DisplayGTK in qapi/ui.json) and just skip the call to
> gd_clipboard_init() unless the option is explicitly enabled ...
> 
> I don't feel like vetoing a compile time option though, so in case you
> prefer to stick with this:
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> take care,
>   Gerd
> 
> 

Thanks Gerd,

I think at least for our packaging purposes we'd rather have it as a configure time option,
so as to not put functionality in the hands of our users that can potentially lock the guest.

Is someone going to queue this, where?

Thanks,

Claudio

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Ani Sinha]

On Wed, Nov 9, 2022 at 6:09 PM Claudio Fontana <cfontana@suse.de> wrote:
>
> On 11/9/22 09:04, Gerd Hoffmann wrote:
>
>
> Thanks Gerd,
>
> I think at least for our packaging purposes we'd rather have it as a configure time option,
> so as to not put functionality in the hands of our users that can potentially lock the guest.
>
> Is someone going to queue this, where?

Unfortunately we are on a hard code freeze at this time for the next
release. It might be better if you can resend the patch again to
remind someone to queue this up once the window opens again after the
release (mid december at the latest).

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Peter Maydell]

On Wed, 9 Nov 2022 at 16:21, Ani Sinha <ani@anisinha.ca> wrote:
>
> On Wed, Nov 9, 2022 at 6:09 PM Claudio Fontana <cfontana@suse.de> wrote:
> >
> > On 11/9/22 09:04, Gerd Hoffmann wrote:
> >
> >
> > Thanks Gerd,
> >
> > I think at least for our packaging purposes we'd rather have it as a configure time option,
> > so as to not put functionality in the hands of our users that can potentially lock the guest.
> >
> > Is someone going to queue this, where?
>
> Unfortunately we are on a hard code freeze at this time for the next
> release. It might be better if you can resend the patch again to
> remind someone to queue this up once the window opens again after the
> release (mid december at the latest).

We are in hard freeze, but that just means "no new features".
This patch is fixing, or at least working around, a bug (i.e. that
QEMU can hang), so it's OK to go in during freeze, assuming the
usual code review etc.

thanks
-- PMM

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Ani Sinha]

On Wed, Nov 9, 2022 at 9:52 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 9 Nov 2022 at 16:21, Ani Sinha <ani@anisinha.ca> wrote:
> >
> > On Wed, Nov 9, 2022 at 6:09 PM Claudio Fontana <cfontana@suse.de> wrote:
> > >
> > > On 11/9/22 09:04, Gerd Hoffmann wrote:
> > >
> > >
> > > Thanks Gerd,
> > >
> > > I think at least for our packaging purposes we'd rather have it as a configure time option,
> > > so as to not put functionality in the hands of our users that can potentially lock the guest.
> > >
> > > Is someone going to queue this, where?
> >
> > Unfortunately we are on a hard code freeze at this time for the next
> > release. It might be better if you can resend the patch again to
> > remind someone to queue this up once the window opens again after the
> > release (mid december at the latest).
>
> We are in hard freeze, but that just means "no new features".
> This patch is fixing, or at least working around, a bug (i.e. that
> QEMU can hang), so it's OK to go in during freeze, assuming the
> usual code review etc.

yeah just realized it's a bugfix. I was about to respond but you beat
me by a few secs :-)

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Jim Fehlig]

I should make myself useful around here on occasion when items are within my 
skill set. But I already struggle to find time for that in the libvirt community 
:-).

On 11/8/22 09:23, Claudio Fontana wrote:
> The GTK Clipboard implementation may cause guest hangs.
> 
> Therefore implement a new configure switch --enable-gtk-clipboard,
> disabled by default, as a meson option.
> 
> Regenerate the meson build options to include it.
> 
> The initialization of the clipboard is gtk.c, as well as the
> compilation of gtk-clipboard.c are now conditional on this new option
> to be set.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>   meson.build                   | 9 +++++++++
>   meson_options.txt             | 7 +++++++
>   scripts/meson-buildoptions.sh | 3 +++
>   ui/gtk.c                      | 2 ++
>   ui/meson.build                | 5 ++++-
>   5 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 1d448272ab..8bb96e5e8c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1243,6 +1243,8 @@ if nettle.found() and gmp.found()
>   endif
>   
>   
> +have_gtk_clipboard = false

Can this be initialized with get_option(), instead of the two calls below?

> +
>   gtk = not_found
>   gtkx11 = not_found
>   vte = not_found
> @@ -1258,12 +1260,18 @@ if not get_option('gtk').auto() or have_system
>                           kwargs: static_kwargs)
>       gtk = declare_dependency(dependencies: [gtk, gtkx11])
>   
> +    have_gtk_clipboard = get_option('gtk_clipboard').enabled()
> +
>       if not get_option('vte').auto() or have_system
>         vte = dependency('vte-2.91',
>                          method: 'pkg-config',
>                          required: get_option('vte'),
>                          kwargs: static_kwargs)
>       endif
> +  else
> +    if get_option('gtk_clipboard').enabled()
> +      error('GTK clipboard requested, but GTK not found')
> +    endif
>     endif
>   endif
>   
> @@ -1842,6 +1850,7 @@ if glusterfs.found()
>   endif
>   config_host_data.set('CONFIG_GTK', gtk.found())
>   config_host_data.set('CONFIG_VTE', vte.found())
> +config_host_data.set('CONFIG_GTK_CLIPBOARD', have_gtk_clipboard)
>   config_host_data.set('CONFIG_LIBATTR', have_old_libattr)
>   config_host_data.set('CONFIG_LIBCAP_NG', libcap_ng.found())
>   config_host_data.set('CONFIG_EBPF', libbpf.found())
> diff --git a/meson_options.txt b/meson_options.txt
> index 66128178bf..4b749ca549 100644
> --- a/meson_options.txt
> +++ b/meson_options.txt
> @@ -219,6 +219,13 @@ option('vnc_sasl', type : 'feature', value : 'auto',
>          description: 'SASL authentication for VNC server')
>   option('vte', type : 'feature', value : 'auto',
>          description: 'vte support for the gtk UI')
> +
> +# GTK Clipboard implementation is disabled by default, since it may cause hangs
> +# of the guest VCPUs. See gitlab issue 1150:
> +# https://gitlab.com/qemu-project/qemu/-/issues/1150
> +
> +option('gtk_clipboard', type: 'feature', value : 'disabled',
> +       description: 'clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)')

'boolean' seems a more appropriate type, but I see 'feature' is common practice 
with these various options. Is gtk_clipboard marked experimental elsewhere? Is 
there a need for the warning text?

>   option('xkbcommon', type : 'feature', value : 'auto',
>          description: 'xkbcommon support')
>   option('zstd', type : 'feature', value : 'auto',
> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> index 2cb0de5601..a542853bfd 100644
> --- a/scripts/meson-buildoptions.sh
> +++ b/scripts/meson-buildoptions.sh
> @@ -93,6 +93,7 @@ meson_options_help() {
>     printf "%s\n" '  glusterfs       Glusterfs block device driver'
>     printf "%s\n" '  gnutls          GNUTLS cryptography support'
>     printf "%s\n" '  gtk             GTK+ user interface'
> +  printf "%s\n" '  gtk-clipboard   clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)'

Same here. None of the other options have such warning. Cant this be treated 
like the others, just another option to be enabled or disabled? Whether or not 
the option works is another matter.

Jim

>     printf "%s\n" '  guest-agent     Build QEMU Guest Agent'
>     printf "%s\n" '  guest-agent-msi Build MSI package for the QEMU Guest Agent'
>     printf "%s\n" '  hax             HAX acceleration support'
> @@ -274,6 +275,8 @@ _meson_option_parse() {
>       --disable-gprof) printf "%s" -Dgprof=false ;;
>       --enable-gtk) printf "%s" -Dgtk=enabled ;;
>       --disable-gtk) printf "%s" -Dgtk=disabled ;;
> +    --enable-gtk-clipboard) printf "%s" -Dgtk_clipboard=enabled ;;
> +    --disable-gtk-clipboard) printf "%s" -Dgtk_clipboard=disabled ;;
>       --enable-guest-agent) printf "%s" -Dguest_agent=enabled ;;
>       --disable-guest-agent) printf "%s" -Dguest_agent=disabled ;;
>       --enable-guest-agent-msi) printf "%s" -Dguest_agent_msi=enabled ;;
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 7ec21f7798..4817623c8f 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2403,7 +2403,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>           opts->u.gtk.show_tabs) {
>           gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
>       }
> +#ifdef CONFIG_GTK_CLIPBOARD
>       gd_clipboard_init(s);
> +#endif /* CONFIG_GTK_CLIPBOARD */
>   }
>   
>   static void early_gtk_display_init(DisplayOptions *opts)
> diff --git a/ui/meson.build b/ui/meson.build
> index ec13949776..c1b137bf33 100644
> --- a/ui/meson.build
> +++ b/ui/meson.build
> @@ -97,7 +97,10 @@ if gtk.found()
>     softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('win32-kbd-hook.c'))
>   
>     gtk_ss = ss.source_set()
> -  gtk_ss.add(gtk, vte, pixman, files('gtk.c', 'gtk-clipboard.c'))
> +  gtk_ss.add(gtk, vte, pixman, files('gtk.c'))
> +  if have_gtk_clipboard
> +    gtk_ss.add(files('gtk-clipboard.c'))
> +  endif
>     gtk_ss.add(when: x11, if_true: files('x_keymap.c'))
>     gtk_ss.add(when: opengl, if_true: files('gtk-gl-area.c'))
>     gtk_ss.add(when: [x11, opengl], if_true: files('gtk-egl.c'))

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Peter Maydell]

On Fri, 18 Nov 2022 at 22:26, Jim Fehlig <jfehlig@suse.com> wrote:
>
> I should make myself useful around here on occasion when items are within my
> skill set. But I already struggle to find time for that in the libvirt community
> :-).
>
> On 11/8/22 09:23, Claudio Fontana wrote:
> > The GTK Clipboard implementation may cause guest hangs.
> >
> > Therefore implement a new configure switch --enable-gtk-clipboard,
> > disabled by default, as a meson option.
> >
> > Regenerate the meson build options to include it.
> >
> > The initialization of the clipboard is gtk.c, as well as the
> > compilation of gtk-clipboard.c are now conditional on this new option
> > to be set.

> > diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
> > index 2cb0de5601..a542853bfd 100644
> > --- a/scripts/meson-buildoptions.sh
> > +++ b/scripts/meson-buildoptions.sh
> > @@ -93,6 +93,7 @@ meson_options_help() {
> >     printf "%s\n" '  glusterfs       Glusterfs block device driver'
> >     printf "%s\n" '  gnutls          GNUTLS cryptography support'
> >     printf "%s\n" '  gtk             GTK+ user interface'
> > +  printf "%s\n" '  gtk-clipboard   clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)'
>
> Same here. None of the other options have such warning. Cant this be treated
> like the others, just another option to be enabled or disabled? Whether or not
> the option works is another matter.

Well, none of the other features have known bugs that cause QEMU to hang...

-- PMM

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Claudio Fontana]

On 11/18/22 23:26, Jim Fehlig wrote:
> I should make myself useful around here on occasion when items are within my 
> skill set. But I already struggle to find time for that in the libvirt community 
> :-).

Thanks for taking a look,
> 
> On 11/8/22 09:23, Claudio Fontana wrote:
>> The GTK Clipboard implementation may cause guest hangs.
>>
>> Therefore implement a new configure switch --enable-gtk-clipboard,
>> disabled by default, as a meson option.
>>
>> Regenerate the meson build options to include it.
>>
>> The initialization of the clipboard is gtk.c, as well as the
>> compilation of gtk-clipboard.c are now conditional on this new option
>> to be set.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>   meson.build                   | 9 +++++++++
>>   meson_options.txt             | 7 +++++++
>>   scripts/meson-buildoptions.sh | 3 +++
>>   ui/gtk.c                      | 2 ++
>>   ui/meson.build                | 5 ++++-
>>   5 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/meson.build b/meson.build
>> index 1d448272ab..8bb96e5e8c 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -1243,6 +1243,8 @@ if nettle.found() and gmp.found()
>>   endif
>>   
>>   
>> +have_gtk_clipboard = false
> 
> Can this be initialized with get_option(), instead of the two calls below?

Yes, I think we can initialize it once here.

> 
>> +
>>   gtk = not_found
>>   gtkx11 = not_found
>>   vte = not_found
>> @@ -1258,12 +1260,18 @@ if not get_option('gtk').auto() or have_system
>>                           kwargs: static_kwargs)
>>       gtk = declare_dependency(dependencies: [gtk, gtkx11])
>>   
>> +    have_gtk_clipboard = get_option('gtk_clipboard').enabled()
>> +

... I'll remove this...

>>       if not get_option('vte').auto() or have_system
>>         vte = dependency('vte-2.91',
>>                          method: 'pkg-config',
>>                          required: get_option('vte'),
>>                          kwargs: static_kwargs)
>>       endif
>> +  else
>> +    if get_option('gtk_clipboard').enabled()
>> +      error('GTK clipboard requested, but GTK not found')
>> +    endif

... and simplify this to an

      elif have_gtk_clipboad


>>     endif
>>   endif
>>   
>> @@ -1842,6 +1850,7 @@ if glusterfs.found()
>>   endif
>>   config_host_data.set('CONFIG_GTK', gtk.found())
>>   config_host_data.set('CONFIG_VTE', vte.found())
>> +config_host_data.set('CONFIG_GTK_CLIPBOARD', have_gtk_clipboard)
>>   config_host_data.set('CONFIG_LIBATTR', have_old_libattr)
>>   config_host_data.set('CONFIG_LIBCAP_NG', libcap_ng.found())
>>   config_host_data.set('CONFIG_EBPF', libbpf.found())
>> diff --git a/meson_options.txt b/meson_options.txt
>> index 66128178bf..4b749ca549 100644
>> --- a/meson_options.txt
>> +++ b/meson_options.txt
>> @@ -219,6 +219,13 @@ option('vnc_sasl', type : 'feature', value : 'auto',
>>          description: 'SASL authentication for VNC server')
>>   option('vte', type : 'feature', value : 'auto',
>>          description: 'vte support for the gtk UI')
>> +
>> +# GTK Clipboard implementation is disabled by default, since it may cause hangs
>> +# of the guest VCPUs. See gitlab issue 1150:
>> +# https://gitlab.com/qemu-project/qemu/-/issues/1150
>> +
>> +option('gtk_clipboard', type: 'feature', value : 'disabled',
>> +       description: 'clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)')
> 
> 'boolean' seems a more appropriate type, but I see 'feature' is common practice 
> with these various options. Is gtk_clipboard marked experimental elsewhere? Is 
> there a need for the warning text?

I did not find anything else to update about the GTK UI docs, at least in docs/ .

The only other place that comes to mind is about/deprecated.rst ,
but that would be about deprecating the feature , not sure we want to do that.

In terms of whether we should warn about the experimental nature of the feature at all,
I think so, as it can hang QEMU.

> 
>>   option('xkbcommon', type : 'feature', value : 'auto',
>>          description: 'xkbcommon support')
>>   option('zstd', type : 'feature', value : 'auto',
>> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
>> index 2cb0de5601..a542853bfd 100644
>> --- a/scripts/meson-buildoptions.sh
>> +++ b/scripts/meson-buildoptions.sh
>> @@ -93,6 +93,7 @@ meson_options_help() {
>>     printf "%s\n" '  glusterfs       Glusterfs block device driver'
>>     printf "%s\n" '  gnutls          GNUTLS cryptography support'
>>     printf "%s\n" '  gtk             GTK+ user interface'
>> +  printf "%s\n" '  gtk-clipboard   clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)'
> 
> Same here. None of the other options have such warning. Cant this be treated 
> like the others, just another option to be enabled or disabled? Whether or not 
> the option works is another matter.

I'd warn the user, packager, etc that enabling this option may hang QEMU.

Hopefully these warnings can be removed as we find another way to collect the necessary GTK events
that does not incur in this bug.

> 
> Jim
> 
>>     printf "%s\n" '  guest-agent     Build QEMU Guest Agent'
>>     printf "%s\n" '  guest-agent-msi Build MSI package for the QEMU Guest Agent'
>>     printf "%s\n" '  hax             HAX acceleration support'
>> @@ -274,6 +275,8 @@ _meson_option_parse() {
>>       --disable-gprof) printf "%s" -Dgprof=false ;;
>>       --enable-gtk) printf "%s" -Dgtk=enabled ;;
>>       --disable-gtk) printf "%s" -Dgtk=disabled ;;
>> +    --enable-gtk-clipboard) printf "%s" -Dgtk_clipboard=enabled ;;
>> +    --disable-gtk-clipboard) printf "%s" -Dgtk_clipboard=disabled ;;
>>       --enable-guest-agent) printf "%s" -Dguest_agent=enabled ;;
>>       --disable-guest-agent) printf "%s" -Dguest_agent=disabled ;;
>>       --enable-guest-agent-msi) printf "%s" -Dguest_agent_msi=enabled ;;
>> diff --git a/ui/gtk.c b/ui/gtk.c
>> index 7ec21f7798..4817623c8f 100644
>> --- a/ui/gtk.c
>> +++ b/ui/gtk.c
>> @@ -2403,7 +2403,9 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>>           opts->u.gtk.show_tabs) {
>>           gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
>>       }
>> +#ifdef CONFIG_GTK_CLIPBOARD
>>       gd_clipboard_init(s);
>> +#endif /* CONFIG_GTK_CLIPBOARD */
>>   }
>>   
>>   static void early_gtk_display_init(DisplayOptions *opts)
>> diff --git a/ui/meson.build b/ui/meson.build
>> index ec13949776..c1b137bf33 100644
>> --- a/ui/meson.build
>> +++ b/ui/meson.build
>> @@ -97,7 +97,10 @@ if gtk.found()
>>     softmmu_ss.add(when: 'CONFIG_WIN32', if_true: files('win32-kbd-hook.c'))
>>   
>>     gtk_ss = ss.source_set()
>> -  gtk_ss.add(gtk, vte, pixman, files('gtk.c', 'gtk-clipboard.c'))
>> +  gtk_ss.add(gtk, vte, pixman, files('gtk.c'))
>> +  if have_gtk_clipboard
>> +    gtk_ss.add(files('gtk-clipboard.c'))
>> +  endif
>>     gtk_ss.add(when: x11, if_true: files('x_keymap.c'))
>>     gtk_ss.add(when: opengl, if_true: files('gtk-gl-area.c'))
>>     gtk_ss.add(when: [x11, opengl], if_true: files('gtk-egl.c'))
> 

Ciao,

Claudio

Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'

[Jim Fehlig]

On 11/21/22 04:24, Claudio Fontana wrote:
> On 11/18/22 23:26, Jim Fehlig wrote:
>> I should make myself useful around here on occasion when items are within my
>> skill set. But I already struggle to find time for that in the libvirt community
>> :-).
> 
> Thanks for taking a look,
>>
>> On 11/8/22 09:23, Claudio Fontana wrote:
>>> The GTK Clipboard implementation may cause guest hangs.
>>>
>>> Therefore implement a new configure switch --enable-gtk-clipboard,
>>> disabled by default, as a meson option.
>>>
>>> Regenerate the meson build options to include it.
>>>
>>> The initialization of the clipboard is gtk.c, as well as the
>>> compilation of gtk-clipboard.c are now conditional on this new option
>>> to be set.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1150
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>    meson.build                   | 9 +++++++++
>>>    meson_options.txt             | 7 +++++++
>>>    scripts/meson-buildoptions.sh | 3 +++
>>>    ui/gtk.c                      | 2 ++
>>>    ui/meson.build                | 5 ++++-
>>>    5 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 1d448272ab..8bb96e5e8c 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -1243,6 +1243,8 @@ if nettle.found() and gmp.found()
>>>    endif
>>>    
>>>    
>>> +have_gtk_clipboard = false
>>
>> Can this be initialized with get_option(), instead of the two calls below?
> 
> Yes, I think we can initialize it once here.
> 
>>
>>> +
>>>    gtk = not_found
>>>    gtkx11 = not_found
>>>    vte = not_found
>>> @@ -1258,12 +1260,18 @@ if not get_option('gtk').auto() or have_system
>>>                            kwargs: static_kwargs)
>>>        gtk = declare_dependency(dependencies: [gtk, gtkx11])
>>>    
>>> +    have_gtk_clipboard = get_option('gtk_clipboard').enabled()
>>> +
> 
> ... I'll remove this...
> 
>>>        if not get_option('vte').auto() or have_system
>>>          vte = dependency('vte-2.91',
>>>                           method: 'pkg-config',
>>>                           required: get_option('vte'),
>>>                           kwargs: static_kwargs)
>>>        endif
>>> +  else
>>> +    if get_option('gtk_clipboard').enabled()
>>> +      error('GTK clipboard requested, but GTK not found')
>>> +    endif
> 
> ... and simplify this to an
> 
>        elif have_gtk_clipboad
> 
> 
>>>      endif
>>>    endif
>>>    
>>> @@ -1842,6 +1850,7 @@ if glusterfs.found()
>>>    endif
>>>    config_host_data.set('CONFIG_GTK', gtk.found())
>>>    config_host_data.set('CONFIG_VTE', vte.found())
>>> +config_host_data.set('CONFIG_GTK_CLIPBOARD', have_gtk_clipboard)
>>>    config_host_data.set('CONFIG_LIBATTR', have_old_libattr)
>>>    config_host_data.set('CONFIG_LIBCAP_NG', libcap_ng.found())
>>>    config_host_data.set('CONFIG_EBPF', libbpf.found())
>>> diff --git a/meson_options.txt b/meson_options.txt
>>> index 66128178bf..4b749ca549 100644
>>> --- a/meson_options.txt
>>> +++ b/meson_options.txt
>>> @@ -219,6 +219,13 @@ option('vnc_sasl', type : 'feature', value : 'auto',
>>>           description: 'SASL authentication for VNC server')
>>>    option('vte', type : 'feature', value : 'auto',
>>>           description: 'vte support for the gtk UI')
>>> +
>>> +# GTK Clipboard implementation is disabled by default, since it may cause hangs
>>> +# of the guest VCPUs. See gitlab issue 1150:
>>> +# https://gitlab.com/qemu-project/qemu/-/issues/1150
>>> +
>>> +option('gtk_clipboard', type: 'feature', value : 'disabled',
>>> +       description: 'clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)')
>>
>> 'boolean' seems a more appropriate type, but I see 'feature' is common practice
>> with these various options. Is gtk_clipboard marked experimental elsewhere? Is
>> there a need for the warning text?
> 
> I did not find anything else to update about the GTK UI docs, at least in docs/ .
> 
> The only other place that comes to mind is about/deprecated.rst ,
> but that would be about deprecating the feature , not sure we want to do that.
> 
> In terms of whether we should warn about the experimental nature of the feature at all,
> I think so, as it can hang QEMU.
> 
>>
>>>    option('xkbcommon', type : 'feature', value : 'auto',
>>>           description: 'xkbcommon support')
>>>    option('zstd', type : 'feature', value : 'auto',
>>> diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
>>> index 2cb0de5601..a542853bfd 100644
>>> --- a/scripts/meson-buildoptions.sh
>>> +++ b/scripts/meson-buildoptions.sh
>>> @@ -93,6 +93,7 @@ meson_options_help() {
>>>      printf "%s\n" '  glusterfs       Glusterfs block device driver'
>>>      printf "%s\n" '  gnutls          GNUTLS cryptography support'
>>>      printf "%s\n" '  gtk             GTK+ user interface'
>>> +  printf "%s\n" '  gtk-clipboard   clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)'
>>
>> Same here. None of the other options have such warning. Cant this be treated
>> like the others, just another option to be enabled or disabled? Whether or not
>> the option works is another matter.
> 
> I'd warn the user, packager, etc that enabling this option may hang QEMU.

I wasn't objecting to the text, only noting the warning is unique. But as Peter 
said, the problem is unique.

> Hopefully these warnings can be removed as we find another way to collect the necessary GTK events
> that does not incur in this bug.

Agreed. And maybe your "loud" warnings will remind the author of such fix to 
remove them :-).

Jim