On Thu, 14 Feb 2019 at 17:04, BALATON Zoltan <bala...@eik.bme.hu> wrote:
>
> On Thu, 14 Feb 2019, Peter Maydell wrote:
> > Currently the handleEvent method will directly call the NSApp
> > sendEvent method for any events that we want to let OSX deal
> > with. When we rearrange the event handling code, the way that
> > we say "let OSX have this event" is going to change. Prepare
> > for that by refactoring so that handleEvent returns a flag
> > indicating whether it consumed the event.
> >
> > Suggested-by: BALATON Zoltan <bala...@eik.bme.hu>
> > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> > ---
> > +static bool bool_with_iothread_lock(BoolCodeBlock block)
> > +{
> > + bool locked = qemu_mutex_iothread_locked();
> > + bool val;
> > +
>
> Git complained about extra white space in the end of the empty line above
> but not sure if it was added during mailing or you have it in the original
> patch.
Almost certainly an error in the original patch. I'll fix it.
> > + if (!locked) {
> > + qemu_mutex_lock_iothread();
> > + }
> > + val = block();
> > + if (!locked) {
> > + qemu_mutex_unlock_iothread();
> > + }
> > + return val;
> > +}
> > +
> > // Mac to QKeyCode conversion
> > const int mac_to_qkeycode_map[] = {
> > [kVK_ANSI_A] = Q_KEY_CODE_A,
> > @@ -320,8 +336,8 @@ - (void) grabMouse;
> > - (void) ungrabMouse;
> > - (void) toggleFullScreen:(id)sender;
> > - (void) handleMonitorInput:(NSEvent *)event;
> > -- (void) handleEvent:(NSEvent *)event;
> > -- (void) handleEventLocked:(NSEvent *)event;
> > +- (bool) handleEvent:(NSEvent *)event;
> > +- (bool) 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
> > @@ -664,15 +680,16 @@ - (void) handleMonitorInput:(NSEvent *)event
> > }
> > }
> >
> > -- (void) handleEvent:(NSEvent *)event
> > +- (bool) handleEvent:(NSEvent *)event
> > {
> > - with_iothread_lock(^{
> > - [self handleEventLocked:event];
> > + return bool_with_iothread_lock(^{
> > + return [self handleEventLocked:event];
> > });
> > }
>
> If this is only ever used for this one method, wouldn't it be easier to
> move locking to the method below (even with some goto after setting a ret
> variable to unlock at the end of the method where now it returns in the
> middle, but maybe it could even be done without goto as the whole code is
> one big switch that can be exited with break and an if that can be skipped
> by a flag)? That may be easier to follow than this method within block
> within method and then you wouldn't need bool_with_iothread_lock and
> neither handleEvent. Unless there's something I'm missing which makes this
> convoluted way needed.
The aim was to avoid having to do changes to handleEvent's code flow
in order to do "run it with the lock held"; it also means that the
invariant "we always unlock the lock" is easy to confirm, whereas
if you do the lock/unlock inside a single handleEvent method you have
to look for whether it was done right in early-exit cases. It's
a bit less of a convincing argument than it was in v1 (where we were
making no changes to handleEvent at all other than wrapping it in a
lock), but I think it still makes sense this way.
> Other than that
> Reviewed-by: BALATON Zoltan <bala...@eik.bme.hu>
thanks
-- PMM
