| Message ID | 20190225102433.22401-2-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | ui/cocoa: Use OSX's main loop | expand |
On Mon, Feb 25, 2019 at 5:24 AM Peter Maydell <peter.maydell@linaro.org> wrote: > The Cocoa UI should run on the main thread; this is enforced > in OSX Mojave. In order to be able to run on the main thread, > we need to make sure we hold the iothread lock whenever we > call into various QEMU UI midlayer functions. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Reviewed-by: Roman Bolshakov <r.bolshakv@yadro.com> > Tested-by: Roman Bolshakov <r.bolshakov@yadro.com> > Message-id: 20190214102816.3393-2-peter.maydell@linaro.org > --- > Changes since v2: add with_iothread_lock wrap to the > qmp_stop()/qmp_cont() calls > --- > ui/cocoa.m | 91 ++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 65 insertions(+), 26 deletions(-) > > diff --git a/ui/cocoa.m b/ui/cocoa.m > index e2567d6946..f1171c4865 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -129,6 +129,21 @@ > NSTextField *pauseLabel; > NSArray * supportedImageFileTypes; > > +// Utility function to run specified code block with iothread lock held > +typedef void (^CodeBlock)(void); > Please don't use blocks. It would lock Mac OS X users into having to use CLang. GCC does not support this non-standard extension. C function pointers and Objective-C's selectors could work in place of blocks. Thank you. <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Feb 25, 2019 at 5:24 AM Peter Maydell <<a href="mailto:peter.maydell@linaro.org">peter.maydell@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The Cocoa UI should run on the main thread; this is enforced<br> in OSX Mojave. In order to be able to run on the main thread,<br> we need to make sure we hold the iothread lock whenever we<br> call into various QEMU UI midlayer functions.<br> <br> Signed-off-by: Peter Maydell <<a href="mailto:peter.maydell@linaro.org" target="_blank">peter.maydell@linaro.org</a>><br> Reviewed-by: Roman Bolshakov <<a href="mailto:r.bolshakv@yadro.com" target="_blank">r.bolshakv@yadro.com</a>><br> Tested-by: Roman Bolshakov <<a href="mailto:r.bolshakov@yadro.com" target="_blank">r.bolshakov@yadro.com</a>><br> Message-id: <a href="mailto:20190214102816.3393-2-peter.maydell@linaro.org" target="_blank">20190214102816.3393-2-peter.maydell@linaro.org</a><br> ---<br> Changes since v2: add with_iothread_lock wrap to the<br> qmp_stop()/qmp_cont() calls<br> ---<br> ui/cocoa.m | 91 ++++++++++++++++++++++++++++++++++++++----------------<br> 1 file changed, 65 insertions(+), 26 deletions(-)<br> <br> diff --git a/ui/cocoa.m b/ui/cocoa.m<br> index e2567d6946..f1171c4865 100644<br> --- a/ui/cocoa.m<br> +++ b/ui/cocoa.m<br> @@ -129,6 +129,21 @@<br> NSTextField *pauseLabel;<br> NSArray * supportedImageFileTypes;<br> <br> +// Utility function to run specified code block with iothread lock held<br> +typedef void (^CodeBlock)(void);<br></blockquote><div><br></div><div>Please don't use blocks. It would lock Mac OS X users into having to use CLang. GCC does not support this non-standard extension.<br></div><div><br></div><div>C function pointers and Objective-C's selectors could work in place of blocks.<br></div><div> </div><div>Thank you.<br></div></div></div>
On Thu, 28 Feb 2019 at 21:53, G 3 <programmingkidx@gmail.com> wrote: > On Mon, Feb 25, 2019 at 5:24 AM Peter Maydell <peter.maydell@linaro.org> wrote: >> +// Utility function to run specified code block with iothread lock held >> +typedef void (^CodeBlock)(void); > > > Please don't use blocks. It would lock Mac OS X users into having to use CLang. GCC does not support this non-standard extension. > > C function pointers and Objective-C's selectors could work in place of blocks. Clang is the standard toolchain for this OS. We have such little developer resource for OSX that I don't think we can reasonably support two toolchains here. Besides, we need blocks to use dispatch_async(), which is a much simpler way to achieve what we're trying to do than the 10.6 compatible approach. Similarly, I am not prepared to continue to support 10.6 at this point: * 10.6's last release was over seven years ago * Apple stopped supporting it four years ago * there have been multiple OSX releases since then * attempting to maintain support for 10.6 would make the code less readable and maintainable for newer OSes we do care about * we do not have either the user base or the developer base on OSX to justify maintaining support for this ancient OS version * it would be out of line with our overall host OS support policy, which targets only the last LTS version or two of distros and OSes I'm sorry if this is inconvenient to you personally, but I don't think it makes any sense as a project to try to support this any longer. thanks -- PMM
diff --git a/ui/cocoa.m b/ui/cocoa.m index e2567d6946..f1171c4865 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -129,6 +129,21 @@ NSTextField *pauseLabel; NSArray * supportedImageFileTypes; +// Utility function to run specified code block with iothread lock held +typedef void (^CodeBlock)(void); + +static void with_iothread_lock(CodeBlock block) +{ + bool locked = qemu_mutex_iothread_locked(); + if (!locked) { + qemu_mutex_lock_iothread(); + } + block(); + if (!locked) { + qemu_mutex_unlock_iothread(); + } +} + // Mac to QKeyCode conversion const int mac_to_qkeycode_map[] = { [kVK_ANSI_A] = Q_KEY_CODE_A, @@ -306,6 +321,7 @@ - (void) ungrabMouse; - (void) toggleFullScreen:(id)sender; - (void) handleMonitorInput:(NSEvent *)event; - (void) handleEvent:(NSEvent *)event; +- (void) handleEventLocked:(NSEvent *)event; - (void) setAbsoluteEnabled:(BOOL)tIsAbsoluteEnabled; /* The state surrounding mouse grabbing is potentially confusing. * isAbsoluteEnabled tracks qemu_input_is_absolute() [ie "is the emulated @@ -649,8 +665,14 @@ - (void) handleMonitorInput:(NSEvent *)event - (void) handleEvent:(NSEvent *)event { - COCOA_DEBUG("QemuCocoaView: handleEvent\n"); + with_iothread_lock(^{ + [self handleEventLocked:event]; + }); +} +- (void) handleEventLocked:(NSEvent *)event +{ + COCOA_DEBUG("QemuCocoaView: handleEvent\n"); int buttons = 0; int keycode = 0; bool mouse_event = false; @@ -945,15 +967,18 @@ - (QEMUScreen) gscreen {return screen;} */ - (void) raiseAllKeys { - int index; const int max_index = ARRAY_SIZE(modifiers_state); - for (index = 0; index < max_index; index++) { - if (modifiers_state[index]) { - modifiers_state[index] = 0; - qemu_input_event_send_key_qcode(dcl->con, index, false); - } - } + with_iothread_lock(^{ + int index; + + for (index = 0; index < max_index; index++) { + if (modifiers_state[index]) { + modifiers_state[index] = 0; + qemu_input_event_send_key_qcode(dcl->con, index, false); + } + } + }); } @end @@ -1178,7 +1203,9 @@ - (void)displayConsole:(id)sender /* Pause the guest */ - (void)pauseQEMU:(id)sender { - qmp_stop(NULL); + with_iothread_lock(^{ + qmp_stop(NULL); + }); [sender setEnabled: NO]; [[[sender menu] itemWithTitle: @"Resume"] setEnabled: YES]; [self displayPause]; @@ -1187,7 +1214,9 @@ - (void)pauseQEMU:(id)sender /* Resume running the guest operating system */ - (void)resumeQEMU:(id) sender { - qmp_cont(NULL); + with_iothread_lock(^{ + qmp_cont(NULL); + }); [sender setEnabled: NO]; [[[sender menu] itemWithTitle: @"Pause"] setEnabled: YES]; [self removePause]; @@ -1215,13 +1244,17 @@ - (void)removePause /* Restarts QEMU */ - (void)restartQEMU:(id)sender { - qmp_system_reset(NULL); + with_iothread_lock(^{ + qmp_system_reset(NULL); + }); } /* Powers down QEMU */ - (void)powerDownQEMU:(id)sender { - qmp_system_powerdown(NULL); + with_iothread_lock(^{ + qmp_system_powerdown(NULL); + }); } /* Ejects the media. @@ -1237,9 +1270,11 @@ - (void)ejectDeviceMedia:(id)sender return; } - Error *err = NULL; - qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding], - false, NULL, false, false, &err); + __block Error *err = NULL; + with_iothread_lock(^{ + qmp_eject(true, [drive cStringUsingEncoding: NSASCIIStringEncoding], + false, NULL, false, false, &err); + }); handleAnyDeviceErrors(err); } @@ -1271,16 +1306,18 @@ - (void)changeDeviceMedia:(id)sender return; } - Error *err = NULL; - qmp_blockdev_change_medium(true, - [drive cStringUsingEncoding: - NSASCIIStringEncoding], - false, NULL, - [file cStringUsingEncoding: - NSASCIIStringEncoding], - true, "raw", - false, 0, - &err); + __block Error *err = NULL; + with_iothread_lock(^{ + qmp_blockdev_change_medium(true, + [drive cStringUsingEncoding: + NSASCIIStringEncoding], + false, NULL, + [file cStringUsingEncoding: + NSASCIIStringEncoding], + true, "raw", + false, 0, + &err); + }); handleAnyDeviceErrors(err); } } @@ -1419,7 +1456,9 @@ - (void)adjustSpeed:(id)sender // get the throttle percentage throttle_pct = [sender tag]; - cpu_throttle_set(throttle_pct); + with_iothread_lock(^{ + cpu_throttle_set(throttle_pct); + }); COCOA_DEBUG("cpu throttling at %d%c\n", cpu_throttle_get_percentage(), '%'); }