qemu-stable
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Jim Fehlig
Subject: Re: [PATCH] gtk: disable GTK Clipboard with a new option 'gtk_clipboard'
Date: Mon, 21 Nov 2022 10:32:26 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]