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

[Jim Fehlig <jfehlig@suse.com>]

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