All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
@ 2022-03-17 12:55 Philippe Mathieu-Daudé
  2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 1/2] ui/cocoa: Code movement Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-03-17 12:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Emanuele Giuseppe Esposito, Peter Maydell,
	Philippe Mathieu-Daudé,
	Gerd Hoffmann, Akihiko Odaki, Paolo Bonzini, Kevin Wolf

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [RFC PATCH-for-7.0 v4 1/2] ui/cocoa: Code movement
  2022-03-17 12:55 [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
@ 2022-03-17 12:55 ` Philippe Mathieu-Daudé
  2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 2/2] ui/cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
  2022-03-19 13:56 ` [RFC PATCH-for-7.0 v4 0/2] cocoa: " Philippe Mathieu-Daudé
  2 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-03-17 12:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Emanuele Giuseppe Esposito, Peter Maydell,
	Philippe Mathieu-Daudé,
	Gerd Hoffmann, Akihiko Odaki, Paolo Bonzini, Kevin Wolf

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Pure code movement in preparation for the next commit.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 ui/cocoa.m | 86 +++++++++++++++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/ui/cocoa.m b/ui/cocoa.m
index cb6e7c41dc..027c3053f7 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -75,6 +75,9 @@ typedef struct {
     int height;
 } QEMUScreen;
 
+@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner>
+@end
+
 static void cocoa_update(DisplayChangeListener *dcl,
                          int x, int y, int w, int h);
 
@@ -109,6 +112,8 @@ static bool allow_events;
 
 static NSInteger cbchangecount = -1;
 static QemuClipboardInfo *cbinfo;
+static QemuClipboardPeer cbpeer;
+static QemuCocoaPasteboardTypeOwner *cbowner;
 static QemuEvent cbevent;
 
 // Utility functions to run specified code block with iothread lock held
@@ -142,6 +147,44 @@ static bool bool_with_iothread_lock(BoolCodeBlock block)
     return val;
 }
 
+/*
+ * The startup process for the OSX/Cocoa UI is complicated, because
+ * OSX insists that the UI runs on the initial main thread, and so we
+ * need to start a second thread which runs the vl.c qemu_main():
+ *
+ * Initial thread:                    2nd thread:
+ * in main():
+ *  create qemu-main thread
+ *  wait on display_init semaphore
+ *                                    call qemu_main()
+ *                                    ...
+ *                                    in cocoa_display_init():
+ *                                     post the display_init semaphore
+ *                                     wait on app_started semaphore
+ *  create application, menus, etc
+ *  enter OSX run loop
+ * in applicationDidFinishLaunching:
+ *  post app_started semaphore
+ *                                     tell main thread to fullscreen if needed
+ *                                    [...]
+ *                                    run qemu main-loop
+ *
+ * We do this in two stages so that we don't do the creation of the
+ * GUI application menus and so on for command line options like --help
+ * where we want to just print text to stdout and exit immediately.
+ */
+
+static void *call_qemu_main(void *opaque)
+{
+    int status;
+
+    COCOA_DEBUG("Second thread: calling qemu_main()\n");
+    status = qemu_main(gArgc, gArgv, *_NSGetEnviron());
+    COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n");
+    [cbowner release];
+    exit(status);
+}
+
 // Mac to QKeyCode conversion
 static const int mac_to_qkeycode_map[] = {
     [kVK_ANSI_A] = Q_KEY_CODE_A,
@@ -1804,9 +1847,6 @@ static void addRemovableDevicesMenuItems(void)
     qapi_free_BlockInfoList(pointerToFree);
 }
 
-@interface QemuCocoaPasteboardTypeOwner : NSObject<NSPasteboardTypeOwner>
-@end
-
 @implementation QemuCocoaPasteboardTypeOwner
 
 - (void)pasteboard:(NSPasteboard *)sender provideDataForType:(NSPasteboardType)type
@@ -1841,8 +1881,6 @@ static void addRemovableDevicesMenuItems(void)
 
 @end
 
-static QemuCocoaPasteboardTypeOwner *cbowner;
-
 static void cocoa_clipboard_notify(Notifier *notifier, void *data);
 static void cocoa_clipboard_request(QemuClipboardInfo *info,
                                     QemuClipboardType type);
@@ -1903,44 +1941,6 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info,
     }
 }
 
-/*
- * The startup process for the OSX/Cocoa UI is complicated, because
- * OSX insists that the UI runs on the initial main thread, and so we
- * need to start a second thread which runs the vl.c qemu_main():
- *
- * Initial thread:                    2nd thread:
- * in main():
- *  create qemu-main thread
- *  wait on display_init semaphore
- *                                    call qemu_main()
- *                                    ...
- *                                    in cocoa_display_init():
- *                                     post the display_init semaphore
- *                                     wait on app_started semaphore
- *  create application, menus, etc
- *  enter OSX run loop
- * in applicationDidFinishLaunching:
- *  post app_started semaphore
- *                                     tell main thread to fullscreen if needed
- *                                    [...]
- *                                    run qemu main-loop
- *
- * We do this in two stages so that we don't do the creation of the
- * GUI application menus and so on for command line options like --help
- * where we want to just print text to stdout and exit immediately.
- */
-
-static void *call_qemu_main(void *opaque)
-{
-    int status;
-
-    COCOA_DEBUG("Second thread: calling qemu_main()\n");
-    status = qemu_main(gArgc, gArgv, *_NSGetEnviron());
-    COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n");
-    [cbowner release];
-    exit(status);
-}
-
 int main (int argc, char **argv) {
     QemuThread thread;
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [RFC PATCH-for-7.0 v4 2/2] ui/cocoa: run qemu_init in the main thread
  2022-03-17 12:55 [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
  2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 1/2] ui/cocoa: Code movement Philippe Mathieu-Daudé
@ 2022-03-17 12:55 ` Philippe Mathieu-Daudé
  2022-03-19 13:56 ` [RFC PATCH-for-7.0 v4 0/2] cocoa: " Philippe Mathieu-Daudé
  2 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-03-17 12:55 UTC (permalink / raw)
  To: qemu-devel
  Cc: Emanuele Giuseppe Esposito, Peter Maydell,
	Philippe Mathieu-Daudé,
	Gerd Hoffmann, Akihiko Odaki, Paolo Bonzini, Kevin Wolf

From: Paolo Bonzini <pbonzini@redhat.com>

Simplify the initialization dance by running qemu_init() in the main
thread before the Cocoa event loop starts.  The cocoa_display_init()
code that is post-applicationDidFinishLaunching: moves to the
application delegate itself, and the secondary thread only runs
the rest of qemu_main(), namely qemu_main_loop() and qemu_cleanup().

This fixes a case where addRemovableDevicesMenuItems() calls
qmp_query_block() while expecting the main thread to still hold
the BQL.  The newly-introduced assertions in the block layer
complain about this.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20220307151004.578069-1-pbonzini@redhat.com>
[PMD: Fixed trivial build failures & rebased]
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 softmmu/main.c |  12 +++--
 ui/cocoa.m     | 123 +++++++++++++++++++++----------------------------
 2 files changed, 60 insertions(+), 75 deletions(-)

diff --git a/softmmu/main.c b/softmmu/main.c
index 639c67ff48..0c4384e980 100644
--- a/softmmu/main.c
+++ b/softmmu/main.c
@@ -39,16 +39,18 @@ int main(int argc, char **argv)
 #endif
 #endif /* CONFIG_SDL */
 
-#ifdef CONFIG_COCOA
-#undef main
-#define main qemu_main
-#endif /* CONFIG_COCOA */
-
+#ifndef CONFIG_COCOA
 int main(int argc, char **argv, char **envp)
 {
+    /*
+     * ui/cocoa.m relies on this being the exact content of main(),
+     * because it has to run everything after qemu_init in a secondary
+     * thread.
+     */
     qemu_init(argc, argv, envp);
     qemu_main_loop();
     qemu_cleanup();
 
     return 0;
 }
+#endif /* CONFIG_COCOA */
diff --git a/ui/cocoa.m b/ui/cocoa.m
index 027c3053f7..4483d5e648 100644
--- a/ui/cocoa.m
+++ b/ui/cocoa.m
@@ -100,14 +100,13 @@ static int last_buttons;
 static int cursor_hide = 1;
 static int left_command_key_enabled = 1;
 static bool swap_opt_cmd;
+static bool full_screen;
+static bool full_grab;
+static bool have_cocoa_ui;
 
-static int gArgc;
-static char **gArgv;
 static bool stretch_video;
 static NSTextField *pauseLabel;
 
-static QemuSemaphore display_init_sem;
-static QemuSemaphore app_started_sem;
 static bool allow_events;
 
 static NSInteger cbchangecount = -1;
@@ -150,39 +149,28 @@ static bool bool_with_iothread_lock(BoolCodeBlock block)
 /*
  * The startup process for the OSX/Cocoa UI is complicated, because
  * OSX insists that the UI runs on the initial main thread, and so we
- * need to start a second thread which runs the vl.c qemu_main():
+ * need to start a second thread which runs qemu_main_loop():
  *
  * Initial thread:                    2nd thread:
  * in main():
- *  create qemu-main thread
- *  wait on display_init semaphore
- *                                    call qemu_main()
- *                                    ...
- *                                    in cocoa_display_init():
- *                                     post the display_init semaphore
- *                                     wait on app_started semaphore
+ *  qemu_init()
  *  create application, menus, etc
  *  enter OSX run loop
  * in applicationDidFinishLaunching:
- *  post app_started semaphore
- *                                     tell main thread to fullscreen if needed
- *                                    [...]
- *                                    run qemu main-loop
- *
- * We do this in two stages so that we don't do the creation of the
- * GUI application menus and so on for command line options like --help
- * where we want to just print text to stdout and exit immediately.
+ *  fullscreen if needed
+ *  create main loop thread
+ *                                    call qemu_main_loop()
  */
 
-static void *call_qemu_main(void *opaque)
+static void *call_qemu_main_loop(void *opaque)
 {
-    int status;
-
-    COCOA_DEBUG("Second thread: calling qemu_main()\n");
-    status = qemu_main(gArgc, gArgv, *_NSGetEnviron());
-    COCOA_DEBUG("Second thread: qemu_main() returned, exiting\n");
+    COCOA_DEBUG("Second thread: calling qemu_main_loop()\n");
+    qemu_mutex_lock_iothread();
+    qemu_main_loop();
+    COCOA_DEBUG("Second thread: qemu_main_loop() returned, exiting\n");
+    qemu_cleanup();
     [cbowner release];
-    exit(status);
+    exit(0);
 }
 
 // Mac to QKeyCode conversion
@@ -627,9 +615,9 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven
          * Don't try to tell QEMU about UI information in the application
          * startup phase -- we haven't yet registered dcl with the QEMU UI
          * layer, and also trying to take the iothread lock would deadlock.
-         * When cocoa_display_init() does register the dcl, the UI layer
-         * will call cocoa_switch(), which will call updateUIInfo, so
-         * we don't lose any information here.
+         * When applicationDidFinishLaunching() does register the dcl, the
+         * UI layer will call cocoa_switch(), which will call updateUIInfo,
+         * so we don't lose any information here.
          */
         return;
     }
@@ -823,9 +811,7 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven
         /*
          * Just let OSX have all events that arrive before
          * applicationDidFinishLaunching.
-         * This avoids a deadlock on the iothread lock, which cocoa_display_init()
-         * will not drop until after the app_started_sem is posted. (In theory
-         * there should not be any such events, but OSX Catalina now emits some.)
+         * This may not be needed anymore?
          */
         return false;
     }
@@ -1321,10 +1307,27 @@ static CGEventRef handleTapEvent(CGEventTapProxy proxy, CGEventType type, CGEven
 
 - (void)applicationDidFinishLaunching: (NSNotification *) note
 {
+    QemuThread main_thread;
+
     COCOA_DEBUG("QemuCocoaAppController: applicationDidFinishLaunching\n");
     allow_events = true;
-    /* Tell cocoa_display_init to proceed */
-    qemu_sem_post(&app_started_sem);
+
+    // register vga output callbacks
+    register_displaychangelistener(&dcl);
+
+    qemu_clipboard_peer_register(&cbpeer);
+
+    qemu_mutex_unlock_iothread();
+    qemu_thread_create(&main_thread, "qemu_main_loop", call_qemu_main_loop,
+                       NULL, QEMU_THREAD_DETACHED);
+
+    if (full_screen) {
+        [NSApp activateIgnoringOtherApps: YES];
+        [self toggleFullScreen: nil];
+    }
+    if (full_grab) {
+        [self setFullGrab: nil];
+    }
 }
 
 - (void)applicationWillTerminate:(NSNotification *)aNotification
@@ -1941,22 +1944,17 @@ static void cocoa_clipboard_request(QemuClipboardInfo *info,
     }
 }
 
-int main (int argc, char **argv) {
-    QemuThread thread;
-
+int main(int argc, char **argv, char **envp)
+{
     COCOA_DEBUG("Entered main()\n");
-    gArgc = argc;
-    gArgv = argv;
 
-    qemu_sem_init(&display_init_sem, 0);
-    qemu_sem_init(&app_started_sem, 0);
-
-    qemu_thread_create(&thread, "qemu_main", call_qemu_main,
-                       NULL, QEMU_THREAD_DETACHED);
-
-    COCOA_DEBUG("Main thread: waiting for display_init_sem\n");
-    qemu_sem_wait(&display_init_sem);
-    COCOA_DEBUG("Main thread: initializing app\n");
+    /* Takes iothread lock, released in applicationDidFinishLaunching:.  */
+    qemu_init(argc, argv, envp);
+    if (!have_cocoa_ui) {
+         qemu_main_loop();
+         qemu_cleanup();
+         return 0;
+    }
 
     NSAutoreleasePool * pool = [[NSAutoreleasePool alloc] init];
 
@@ -1979,6 +1977,9 @@ int main (int argc, char **argv) {
     add_console_menu_entries();
     addRemovableDevicesMenuItems();
 
+    qemu_event_init(&cbevent, false);
+    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
+
     // Create an Application controller
     QemuCocoaAppController *appController = [[QemuCocoaAppController alloc] init];
     [NSApp setDelegate:appController];
@@ -2071,24 +2072,13 @@ static void cocoa_refresh(DisplayChangeListener *dcl)
 static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
 {
     COCOA_DEBUG("qemu_cocoa: cocoa_display_init\n");
+    have_cocoa_ui = 1;
 
-    /* Tell main thread to go ahead and create the app and enter the run loop */
-    qemu_sem_post(&display_init_sem);
-    qemu_sem_wait(&app_started_sem);
-    COCOA_DEBUG("cocoa_display_init: app start completed\n");
-
-    QemuCocoaAppController *controller = (QemuCocoaAppController *)[[NSApplication sharedApplication] delegate];
-    /* if fullscreen mode is to be used */
     if (opts->has_full_screen && opts->full_screen) {
-        dispatch_async(dispatch_get_main_queue(), ^{
-            [NSApp activateIgnoringOtherApps: YES];
-            [controller toggleFullScreen: nil];
-        });
+        full_screen = 1;
     }
     if (opts->u.cocoa.has_full_grab && opts->u.cocoa.full_grab) {
-        dispatch_async(dispatch_get_main_queue(), ^{
-            [controller setFullGrab: nil];
-        });
+        full_grab = 1;
     }
 
     if (opts->has_show_cursor && opts->show_cursor) {
@@ -2101,13 +2091,6 @@ static void cocoa_display_init(DisplayState *ds, DisplayOptions *opts)
     if (opts->u.cocoa.has_left_command_key && !opts->u.cocoa.left_command_key) {
         left_command_key_enabled = 0;
     }
-
-    // register vga output callbacks
-    register_displaychangelistener(&dcl);
-
-    qemu_event_init(&cbevent, false);
-    cbowner = [[QemuCocoaPasteboardTypeOwner alloc] init];
-    qemu_clipboard_peer_register(&cbpeer);
 }
 
 static QemuDisplay qemu_display_cocoa = {
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
  2022-03-17 12:55 [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
  2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 1/2] ui/cocoa: Code movement Philippe Mathieu-Daudé
  2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 2/2] ui/cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
@ 2022-03-19 13:56 ` Philippe Mathieu-Daudé
  2022-03-19 14:15   ` Akihiko Odaki
  2022-03-21  9:14   ` Paolo Bonzini
  2 siblings, 2 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-03-19 13:56 UTC (permalink / raw)
  To: qemu-devel, Peter Maydell, Paolo Bonzini, Akihiko Odaki
  Cc: Emanuele Giuseppe Esposito, Kevin Wolf, Gerd Hoffmann,
	Philippe Mathieu-Daudé

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(-)
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
  2022-03-19 13:56 ` [RFC PATCH-for-7.0 v4 0/2] cocoa: " Philippe Mathieu-Daudé
@ 2022-03-19 14:15   ` Akihiko Odaki
  2022-03-21  9:14   ` Paolo Bonzini
  1 sibling, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2022-03-19 14:15 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, Peter Maydell, Paolo Bonzini
  Cc: Emanuele Giuseppe Esposito, Kevin Wolf, Gerd Hoffmann,
	Philippe Mathieu-Daudé

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(-)
>>
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
  2022-03-19 13:56 ` [RFC PATCH-for-7.0 v4 0/2] cocoa: " Philippe Mathieu-Daudé
  2022-03-19 14:15   ` Akihiko Odaki
@ 2022-03-21  9:14   ` Paolo Bonzini
  2022-03-21  9:33     ` Akihiko Odaki
  1 sibling, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2022-03-21  9:14 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, Peter Maydell, Akihiko Odaki
  Cc: Emanuele Giuseppe Esposito, Kevin Wolf, Gerd Hoffmann,
	Philippe Mathieu-Daudé

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread
  2022-03-21  9:14   ` Paolo Bonzini
@ 2022-03-21  9:33     ` Akihiko Odaki
  0 siblings, 0 replies; 7+ messages in thread
From: Akihiko Odaki @ 2022-03-21  9:33 UTC (permalink / raw)
  To: Paolo Bonzini, Philippe Mathieu-Daudé, qemu-devel, Peter Maydell
  Cc: Emanuele Giuseppe Esposito, Kevin Wolf, Gerd Hoffmann,
	Philippe Mathieu-Daudé

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-03-21  9:35 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-17 12:55 [RFC PATCH-for-7.0 v4 0/2] cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 1/2] ui/cocoa: Code movement Philippe Mathieu-Daudé
2022-03-17 12:55 ` [RFC PATCH-for-7.0 v4 2/2] ui/cocoa: run qemu_init in the main thread Philippe Mathieu-Daudé
2022-03-19 13:56 ` [RFC PATCH-for-7.0 v4 0/2] cocoa: " Philippe Mathieu-Daudé
2022-03-19 14:15   ` Akihiko Odaki
2022-03-21  9:14   ` Paolo Bonzini
2022-03-21  9:33     ` Akihiko Odaki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.