Patchwork git bisect results

login
register
mail settings
Submitter Jan Kiszka
Date Jan. 30, 2012, 2:48 p.m.
Message ID <4F26ADBB.3060403@siemens.com>
Download mbox | patch
Permalink /patch/138601/
State New
Headers show

Comments

Jan Kiszka - Jan. 30, 2012, 2:48 p.m.
On 2012-01-30 15:17, Erik Rull wrote:
> 
> 
> 
> On January 30, 2012 at 2:48 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> On 2012-01-30 14:17, Erik Rull wrote:
>>>
>>>
>>>
>>> On January 30, 2012 at 12:52 PM Jan Kiszka <jan.kiszka@siemens.com>
> wrote:
>>>
>>>> On 2012-01-30 12:34, Erik Rull wrote:
>>>>> Hi Jan,
>>>>>
>>>>> I'm sorry, but this does not solve my issue. I applied the patch and
>>>>> crosschecked that the resulting file looks fine.
>>>>>
>>>>> The final function looks like:
>>>>>
>>>>> static void sdl_grab_start(void)
>>>>> {
>>>>> /*
>>>>> * If the application is not active, do not try to enter grab state.
>>> This
>>>>> * prevents 'SDL_WM_GrabInput(SDL_GRAB_ON)' from blocking all the
>>>>> * application (SDL bug).
>>>>> */
>>>>> if (!(SDL_GetAppState() & SDL_APPINPUTFOCUS)) {
>>>>> return;
>>>>> }
>>>>> if (guest_cursor) {
>>>>> SDL_SetCursor(guest_sprite);
>>>>> if (!kbd_mouse_is_absolute() && !absolute_enabled)
>>>>> SDL_WarpMouse(guest_x, guest_y);
>>>>> } else
>>>>> sdl_hide_cursor();
>>>>> SDL_WM_GrabInput(SDL_GRAB_ON);
>>>>> gui_grab = 1;
>>>>> sdl_update_caption();
>>>>> }
>>>>
>>>> That makes no sense as gui_grab must be 1 now. Please retry your
>>>> previous instrumentation.
>>>>
>>>> Thanks,
>>>> Jan
>>>>
>>>
>>> You're right. So I added the instrumentation again.
>>>
>>> Still looks strange.
>>>
>>> So I added into the sdl_grab_start() a printf.
>>> Wow - a lot of output!
>>> This pointed me to all other sdl_grab_start() calls (and in additon to
> that
>>> all sdl_grab_end() calls).
>>>
>>> And here are the results of the qemu voting :-)
>>>
>>> I already assigned a usable name to the printf output that is directly
> one
>>> line above the corresponding sdl_grab_*() call, so you should be able
> to
>>> find this easily in your code as well.
>>>
>>> The huge number of recurring printf's are:
>>>
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>> sdl_grab_start() called from absolute_mouse_grab()
>>> sdl_grab_end() called from handle_activation()
>>>
>>> Any idea how to proceed?
>>>
>>> Maybe the first two if-statements in handle_activation() cause the
> problem?
>>> Because there the two given functions are called in sequence if both
>>> if-clauses are valid one after the other. Maybe the first one sets the
>>> state so that the second if is valid, too. Maybe a simple else if
> solves
>>> the issue?
>>
>> ev->active.gain makes both clauses mutually exclusive - unless someone
>> messes with the memory of the event object.
>>
>>> I'm not familiar with the variables that are checked here, so
>>> it's just a guess.
>>
>> So handle_activation() is called directly after absolute_mouse_grab(),
>> and the reported event contains
>>
>> state = SDL_APPINPUTFOCUS
>> gain = 0
>> (please validate!)
>>
>> That would mean we are constantly losing the input focus again after
>> trying to gain it via SDL_WM_GrabInput. Weird.
>>
>> What's the call chain for absolute_mouse_grab()?
>>
>> Jan
>>
>> --
>> Siemens AG, Corporate Technology, CT T DE IT 1
>> Corporate Competence Center Embedded Linux
> 
> 
> Here the results:
> 
> Handle Activation 0: at function start (before the first if)
> Handle Activation 1: between the first and the second if
> Handle Activation 2: after the second if
> 
> The output is formatted like:
> printf("Handle Activation 0: %d %d %d %d\n",gui_grab,(ev->active.state ==
> SDL_APPINPUTFOCUS),ev->active.gain,gui_fullscreen);
> 
> Handle Activation 0: 0 1 1 0
> Handle Activation 1: 0 1 1 0
> absolute_mouse_grab() called from handle_activation()
> sdl_grab_start() called from absolute_mouse_grab()
> Handle Activation 2: 1 1 1 0
> Handle Activation 0: 1 1 0 0
> sdl_grab_end() called from handle_activation()
> Handle Activation 1: 0 1 0 0
> Handle Activation 2: 0 1 0 0
> Handle Activation 0: 0 1 1 0
> Handle Activation 1: 0 1 1 0
> absolute_mouse_grab() called from handle_activation()
> sdl_grab_start() called from absolute_mouse_grab()
> Handle Activation 2: 1 1 1 0
> Handle Activation 0: 1 1 0 0
> sdl_grab_end() called from handle_activation()
> Handle Activation 1: 0 1 0 0
> Handle Activation 2: 0 1 0 0
> Handle Activation 0: 0 1 1 0
> Handle Activation 1: 0 1 1 0
> absolute_mouse_grab() called from handle_activation()
> sdl_grab_start() called from absolute_mouse_grab()
> Handle Activation 2: 1 1 1 0
> 
> The gain seems to toggle between the successive calls of
> handle_activation...
> Next steps?

Try this:


I'm wondering what this code is branch should do anyway. If we grabbed
the input we can't unwillingly lose it. But that's just a theory for
now.

Jan
Erik Rull - Jan. 31, 2012, 8:31 a.m.
On January 30, 2012 at 3:48 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2012-01-30 15:17, Erik Rull wrote:
> >
> >
> >
> > On January 30, 2012 at 2:48 PM Jan Kiszka <jan.kiszka@siemens.com>
wrote:
> >
> >> On 2012-01-30 14:17, Erik Rull wrote:
> >>>
> >>>
> >>>
> >>> On January 30, 2012 at 12:52 PM Jan Kiszka <jan.kiszka@siemens.com>
> > wrote:
> >>>
> >>>> On 2012-01-30 12:34, Erik Rull wrote:
> >>>>> Hi Jan,
> >>>>>
> >>>>> I'm sorry, but this does not solve my issue. I applied the patch
and
> >>>>> crosschecked that the resulting file looks fine.
> >>>>>
> >>>>> The final function looks like:
> >>>>>
> >>>>> static void sdl_grab_start(void)
> >>>>> {
> >>>>> /*
> >>>>> * If the application is not active, do not try to enter grab state.
> >>> This
> >>>>> * prevents 'SDL_WM_GrabInput(SDL_GRAB_ON)' from blocking all the
> >>>>> * application (SDL bug).
> >>>>> */
> >>>>> if (!(SDL_GetAppState() & SDL_APPINPUTFOCUS)) {
> >>>>> return;
> >>>>> }
> >>>>> if (guest_cursor) {
> >>>>> SDL_SetCursor(guest_sprite);
> >>>>> if (!kbd_mouse_is_absolute() && !absolute_enabled)
> >>>>> SDL_WarpMouse(guest_x, guest_y);
> >>>>> } else
> >>>>> sdl_hide_cursor();
> >>>>> SDL_WM_GrabInput(SDL_GRAB_ON);
> >>>>> gui_grab = 1;
> >>>>> sdl_update_caption();
> >>>>> }
> >>>>
> >>>> That makes no sense as gui_grab must be 1 now. Please retry your
> >>>> previous instrumentation.
> >>>>
> >>>> Thanks,
> >>>> Jan
> >>>>
> >>>
> >>> You're right. So I added the instrumentation again.
> >>>
> >>> Still looks strange.
> >>>
> >>> So I added into the sdl_grab_start() a printf.
> >>> Wow - a lot of output!
> >>> This pointed me to all other sdl_grab_start() calls (and in additon
to
> > that
> >>> all sdl_grab_end() calls).
> >>>
> >>> And here are the results of the qemu voting :-)
> >>>
> >>> I already assigned a usable name to the printf output that is
directly
> > one
> >>> line above the corresponding sdl_grab_*() call, so you should be able
> > to
> >>> find this easily in your code as well.
> >>>
> >>> The huge number of recurring printf's are:
> >>>
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>> sdl_grab_start() called from absolute_mouse_grab()
> >>> sdl_grab_end() called from handle_activation()
> >>>
> >>> Any idea how to proceed?
> >>>
> >>> Maybe the first two if-statements in handle_activation() cause the
> > problem?
> >>> Because there the two given functions are called in sequence if both
> >>> if-clauses are valid one after the other. Maybe the first one sets
the
> >>> state so that the second if is valid, too. Maybe a simple else if
> > solves
> >>> the issue?
> >>
> >> ev->active.gain makes both clauses mutually exclusive - unless someone
> >> messes with the memory of the event object.
> >>
> >>> I'm not familiar with the variables that are checked here, so
> >>> it's just a guess.
> >>
> >> So handle_activation() is called directly after absolute_mouse_grab(),
> >> and the reported event contains
> >>
> >> state = SDL_APPINPUTFOCUS
> >> gain = 0
> >> (please validate!)
> >>
> >> That would mean we are constantly losing the input focus again after
> >> trying to gain it via SDL_WM_GrabInput. Weird.
> >>
> >> What's the call chain for absolute_mouse_grab()?
> >>
> >> Jan
> >>
> >> --
> >> Siemens AG, Corporate Technology, CT T DE IT 1
> >> Corporate Competence Center Embedded Linux
> >
> >
> > Here the results:
> >
> > Handle Activation 0: at function start (before the first if)
> > Handle Activation 1: between the first and the second if
> > Handle Activation 2: after the second if
> >
> > The output is formatted like:
> > printf("Handle Activation 0: %d %d %d %d\n",gui_grab,(ev->active.state
==
> > SDL_APPINPUTFOCUS),ev->active.gain,gui_fullscreen);
> >
> > Handle Activation 0: 0 1 1 0
> > Handle Activation 1: 0 1 1 0
> > absolute_mouse_grab() called from handle_activation()
> > sdl_grab_start() called from absolute_mouse_grab()
> > Handle Activation 2: 1 1 1 0
> > Handle Activation 0: 1 1 0 0
> > sdl_grab_end() called from handle_activation()
> > Handle Activation 1: 0 1 0 0
> > Handle Activation 2: 0 1 0 0
> > Handle Activation 0: 0 1 1 0
> > Handle Activation 1: 0 1 1 0
> > absolute_mouse_grab() called from handle_activation()
> > sdl_grab_start() called from absolute_mouse_grab()
> > Handle Activation 2: 1 1 1 0
> > Handle Activation 0: 1 1 0 0
> > sdl_grab_end() called from handle_activation()
> > Handle Activation 1: 0 1 0 0
> > Handle Activation 2: 0 1 0 0
> > Handle Activation 0: 0 1 1 0
> > Handle Activation 1: 0 1 1 0
> > absolute_mouse_grab() called from handle_activation()
> > sdl_grab_start() called from absolute_mouse_grab()
> > Handle Activation 2: 1 1 1 0
> >
> > The gain seems to toggle between the successive calls of
> > handle_activation...
> > Next steps?
>
> Try this:
>
> diff --git a/ui/sdl.c b/ui/sdl.c
> index 73e5839..c3fe80d 100644
> --- a/ui/sdl.c
> +++ b/ui/sdl.c
> @@ -828,10 +828,6 @@ static void handle_mousebutton(DisplayState *ds,
SDL_Event *ev)
> 
>  static void handle_activation(DisplayState *ds, SDL_Event *ev)
>  {
> -    if (gui_grab && ev->active.state == SDL_APPINPUTFOCUS &&
> -        !ev->active.gain && !gui_fullscreen) {
> -        sdl_grab_end();
> -    }
>      if (!gui_grab && ev->active.gain && is_graphic_console() &&
>          (kbd_mouse_is_absolute() || absolute_enabled)) {
>          absolute_mouse_grab();
>
> I'm wondering what this code is branch should do anyway. If we grabbed
> the input we can't unwillingly lose it. But that's just a theory for
> now.
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux


That's it. Fine from my side now!

Best regards,

Erik

Patch

diff --git a/ui/sdl.c b/ui/sdl.c
index 73e5839..c3fe80d 100644
--- a/ui/sdl.c
+++ b/ui/sdl.c
@@ -828,10 +828,6 @@  static void handle_mousebutton(DisplayState *ds, SDL_Event *ev)
 
 static void handle_activation(DisplayState *ds, SDL_Event *ev)
 {
-    if (gui_grab && ev->active.state == SDL_APPINPUTFOCUS &&
-        !ev->active.gain && !gui_fullscreen) {
-        sdl_grab_end();
-    }
     if (!gui_grab && ev->active.gain && is_graphic_console() &&
         (kbd_mouse_is_absolute() || absolute_enabled)) {
         absolute_mouse_grab();