Re: [Qemu-devel] [PULL 1/7] ui/cocoa: Ensure we have the iothread lock when calling into QEMU

[Philippe Mathieu-Daudé]

Hi Peter,

On 3/4/19 5:49 PM, Peter Maydell 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 <address@hidden>
> Reviewed-by: Roman Bolshakov <address@hidden>
> Tested-by: Roman Bolshakov <address@hidden>

I got surprised by your 2 Message-id lines (same with other patches from
this pull request):

> Message-id: address@hidden

^ v3

> Message-id: address@hidden

^ v2

> ---
>  ui/cocoa.m | 91 ++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 65 insertions(+), 26 deletions(-)
> 
> diff --git a/ui/cocoa.m b/ui/cocoa.m
> index e2567d69466..f1171c48654 100644
> --- a/ui/cocoa.m
> +++ b/ui/cocoa.m
> @@ -129,6 +129,21 @@ bool stretch_video;
>  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 @@ static void handleAnyDeviceErrors(Error * err)
>  - (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 @@ QemuCocoaView *cocoaView;
>  
>  - (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 @@ QemuCocoaView *cocoaView;
>   */
>  - (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 @@ QemuCocoaView *cocoaView;
>  /* 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 @@ QemuCocoaView *cocoaView;
>  /* 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 @@ QemuCocoaView *cocoaView;
>  /* 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 @@ QemuCocoaView *cocoaView;
>          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 @@ QemuCocoaView *cocoaView;
>              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 @@ QemuCocoaView *cocoaView;
>      // 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(), 
> '%');
>  }
>  
>