[RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread

Philippe Mathieu-Daudé posted 2 patches 2 years ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220317125534.38706-1-philippe.mathieu.daude@gmail.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, "Philippe Mathieu-Daudé" <f4bug@amsat.org>, Akihiko Odaki <akihiko.odaki@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>
softmmu/main.c |  12 ++--
ui/cocoa.m     | 161 ++++++++++++++++++++++---------------------------
2 files changed, 79 insertions(+), 94 deletions(-)
[RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
Posted by Philippe Mathieu-Daudé 2 years ago
From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Posting v4 in case someone want to iterate.

Pending issue raised by Akihiko Odaki:

* this actually breaks the "runas" option with ui/cocoa.

  [+NSApplication sharedApplication] calls issetugid() to see if
  setgid() or setuid() is called before and calls exit() if it evaluates
  true. It does not evaluate true without this patch since setgid() and
  setuid() are called after [+NSApplication sharedApplication]. This
  patch, however, changes the order and triggers the check.

  There are two options to solve the problem:
  1. Move setgid and setuid calls after [+NSApplication
  sharedApplication] to let NSApplication initialize as the original
  user.
  2. Do: [[NSUserDefaults standardUserDefaults] setBool:YES
  forKey:@"_NSAppAllowsNonTrustedUGID"]

  Option 2 would be preferred in terms of practicality since nobody
  would want to initialize NSApplication as the original user (usually
  superuser). However, _NSAppAllowsNonTrustedUGID is not documented by
  Apple.

* Oudated comment in main():

 1970  /*
 1971   * Create the menu entries which depend on QEMU state (for consoles
 1972   * and removeable devices). These make calls back into QEMU functions,
 1973   * which is OK because at this point we know that the second thread
 1974   * holds the iothread lock and is synchronously waiting for us to
 1975   * finish.
 1976   */

(https://marc.info/?l=qemu-devel&m=164752136410805)

Since v3:
- Move qemu_event_init before cbowner alloc
- Reduce main_thread scope to applicationDidFinishLaunching
- Updated updateUIInfo() comment
  (s/cocoa_display_init/applicationDidFinishLaunching)

Since v2:
- Extracted code movement in preliminary patch

v3: https://lore.kernel.org/qemu-devel/20220317115644.37276-1-philippe.mathieu.daude@gmail.com/
v2: https://lore.kernel.org/qemu-devel/20220316160300.85438-1-philippe.mathieu.daude@gmail.com/
v1: https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonzini@redhat.com/

Paolo Bonzini (1):
  ui/cocoa: run qemu_init in the main thread

Philippe Mathieu-Daudé (1):
  ui/cocoa: Code movement

 softmmu/main.c |  12 ++--
 ui/cocoa.m     | 161 ++++++++++++++++++++++---------------------------
 2 files changed, 79 insertions(+), 94 deletions(-)

-- 
2.34.1


Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
Posted by Philippe Mathieu-Daudé 2 years ago
Hi Akihiko, Paolo, Peter.

On 17/3/22 13:55, Philippe Mathieu-Daudé wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
> Posting v4 in case someone want to iterate.
> 
> Pending issue raised by Akihiko Odaki:
> 
> * this actually breaks the "runas" option with ui/cocoa.
> 
>    [+NSApplication sharedApplication] calls issetugid() to see if
>    setgid() or setuid() is called before and calls exit() if it evaluates
>    true. It does not evaluate true without this patch since setgid() and
>    setuid() are called after [+NSApplication sharedApplication]. This
>    patch, however, changes the order and triggers the check.
> 
>    There are two options to solve the problem:
>    1. Move setgid and setuid calls after [+NSApplication
>    sharedApplication] to let NSApplication initialize as the original
>    user.

Akihiko, could you send a patch?

>    2. Do: [[NSUserDefaults standardUserDefaults] setBool:YES
>    forKey:@"_NSAppAllowsNonTrustedUGID"]
> 
>    Option 2 would be preferred in terms of practicality since nobody
>    would want to initialize NSApplication as the original user (usually
>    superuser). However, _NSAppAllowsNonTrustedUGID is not documented by
>    Apple.

What are your views on this problem for 7.0-rc1? Keep modifying cocoa
UI? Disable block layer assertions? Only disable them for Darwin?

> * Oudated comment in main():
> 
>   1970  /*
>   1971   * Create the menu entries which depend on QEMU state (for consoles
>   1972   * and removeable devices). These make calls back into QEMU functions,
>   1973   * which is OK because at this point we know that the second thread
>   1974   * holds the iothread lock and is synchronously waiting for us to
>   1975   * finish.
>   1976   */
> 
> (https://marc.info/?l=qemu-devel&m=164752136410805)
> 
> Since v3:
> - Move qemu_event_init before cbowner alloc
> - Reduce main_thread scope to applicationDidFinishLaunching
> - Updated updateUIInfo() comment
>    (s/cocoa_display_init/applicationDidFinishLaunching)
> 
> Since v2:
> - Extracted code movement in preliminary patch
> 
> v3: https://lore.kernel.org/qemu-devel/20220317115644.37276-1-philippe.mathieu.daude@gmail.com/
> v2: https://lore.kernel.org/qemu-devel/20220316160300.85438-1-philippe.mathieu.daude@gmail.com/
> v1: https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonzini@redhat.com/
> 
> Paolo Bonzini (1):
>    ui/cocoa: run qemu_init in the main thread
> 
> Philippe Mathieu-Daudé (1):
>    ui/cocoa: Code movement
> 
>   softmmu/main.c |  12 ++--
>   ui/cocoa.m     | 161 ++++++++++++++++++++++---------------------------
>   2 files changed, 79 insertions(+), 94 deletions(-)
> 


Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
Posted by Paolo Bonzini 2 years ago
On 3/19/22 14:56, Philippe Mathieu-Daudé wrote:
>>    1. Move setgid and setuid calls after [+NSApplication
>>    sharedApplication] to let NSApplication initialize as the original
>>    user.

Another possibility is to move the code up to "[QemuApplication 
sharedApplication]" from main() to cocoa_display_init().

Paolo

Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
Posted by Akihiko Odaki 2 years ago
On 2022/03/21 18:14, Paolo Bonzini wrote:
> On 3/19/22 14:56, Philippe Mathieu-Daudé wrote:
>>>    1. Move setgid and setuid calls after [+NSApplication
>>>    sharedApplication] to let NSApplication initialize as the original
>>>    user.
> 
> Another possibility is to move the code up to "[QemuApplication 
> sharedApplication]" from main() to cocoa_display_init().
> 
> Paolo

I'm for moving everything except [NSApp run] to cocoa_display_init().

This series moved Cocoa display initialization code to 
[-QemuCocoaApplicatinController applicationDidFinishLaunching:] to keep 
the initialization order for Cocoa, but it is unlikely that doing 
initialization things before [-QemuCocoaApplicatinController 
applicationDidFinishLaunching:] would cause problems. (The only Apple 
thing affected by the change would be dispatch_sync, which is rarely 
used.) The code movement is rather intrusive for QEMU as found with this 
"runas" problem.

Doing Cocoa initialization things in [-QemuCocoaApplicatinController 
applicationDidFinishLaunching:] and after the method call can be even 
dangerous as it would modify the state of Cocoa, which is already 
running. "Create menus in iothread" patch series actually triggered such 
a problem and required careful coding:
https://patchew.org/QEMU/20220321041043.24112-1-akihiko.odaki@gmail.com/

The code movement also requires more code to keep the content of 
DisplayOptions and to have the definition of 
[-QemuCocoaApplicatinController applicationDidFinishLaunching:]. It 
would be simply nice if we could remove them.

Regards,
Akihiko Odaki

Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
Posted by Akihiko Odaki 2 years ago
On 2022/03/19 22:56, Philippe Mathieu-Daudé wrote:
> Hi Akihiko, Paolo, Peter.
> 
> On 17/3/22 13:55, Philippe Mathieu-Daudé wrote:
>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Posting v4 in case someone want to iterate.
>>
>> Pending issue raised by Akihiko Odaki:
>>
>> * this actually breaks the "runas" option with ui/cocoa.
>>
>>    [+NSApplication sharedApplication] calls issetugid() to see if
>>    setgid() or setuid() is called before and calls exit() if it evaluates
>>    true. It does not evaluate true without this patch since setgid() and
>>    setuid() are called after [+NSApplication sharedApplication]. This
>>    patch, however, changes the order and triggers the check.
>>
>>    There are two options to solve the problem:
>>    1. Move setgid and setuid calls after [+NSApplication
>>    sharedApplication] to let NSApplication initialize as the original
>>    user.
> 
> Akihiko, could you send a patch?

Do you mean a patch for option 1?

> 
>>    2. Do: [[NSUserDefaults standardUserDefaults] setBool:YES
>>    forKey:@"_NSAppAllowsNonTrustedUGID"]
>>
>>    Option 2 would be preferred in terms of practicality since nobody
>>    would want to initialize NSApplication as the original user (usually
>>    superuser). However, _NSAppAllowsNonTrustedUGID is not documented by
>>    Apple.
> 
> What are your views on this problem for 7.0-rc1? Keep modifying cocoa
> UI? Disable block layer assertions? Only disable them for Darwin?

I think we should disable block layer assertions. It is not a change 
visible to user and its value is more apparent in development.
We can preserve most of its benefit if we restore the assertions 
immediately after 7.0 release and let them work during the next 
development cycle.

If it is not preferred, we can apply the following change as a 
less-intrusive alternative:
https://patchew.org/QEMU/20220307134946.61407-1-akihiko.odaki@gmail.com/

The change faced objections as it uses Cocoa APIs from iothread. It is 
still in accordance with Cocoa's API convention and the only negative 
effect is that it can confuse developers. It is not affecting users and 
we can also minimize the possibility of confusion by immediately 
following with this "qemu_init in the main thread" patch after 7.0 release.

Regards,
Akihiko Odaki

> 
>> * Oudated comment in main():
>>
>>   1970  /*
>>   1971   * Create the menu entries which depend on QEMU state (for 
>> consoles
>>   1972   * and removeable devices). These make calls back into QEMU 
>> functions,
>>   1973   * which is OK because at this point we know that the second 
>> thread
>>   1974   * holds the iothread lock and is synchronously waiting for us to
>>   1975   * finish.
>>   1976   */
>>
>> (https://marc.info/?l=qemu-devel&m=164752136410805)
>>
>> Since v3:
>> - Move qemu_event_init before cbowner alloc
>> - Reduce main_thread scope to applicationDidFinishLaunching
>> - Updated updateUIInfo() comment
>>    (s/cocoa_display_init/applicationDidFinishLaunching)
>>
>> Since v2:
>> - Extracted code movement in preliminary patch
>>
>> v3: 
>> https://lore.kernel.org/qemu-devel/20220317115644.37276-1-philippe.mathieu.daude@gmail.com/ 
>>
>> v2: 
>> https://lore.kernel.org/qemu-devel/20220316160300.85438-1-philippe.mathieu.daude@gmail.com/ 
>>
>> v1: 
>> https://lore.kernel.org/qemu-devel/20220307151004.578069-1-pbonzini@redhat.com/ 
>>
>>
>> Paolo Bonzini (1):
>>    ui/cocoa: run qemu_init in the main thread
>>
>> Philippe Mathieu-Daudé (1):
>>    ui/cocoa: Code movement
>>
>>   softmmu/main.c |  12 ++--
>>   ui/cocoa.m     | 161 ++++++++++++++++++++++---------------------------
>>   2 files changed, 79 insertions(+), 94 deletions(-)
>>
>