Re: winex11.drv: Ensure that wintab xinput_handle is not NULL.

[John Klehm]

On Mon, Feb 28, 2011 at 8:50 PM, Peter Urbanec <winehq.org(a)urbanec.net> wrote:
> ---
>  dlls/winex11.drv/wintab.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)

Nice find on this code path being able to run without loading xinput.
I have a couple comments:


You don't need to guard inside GetCurrentPacket.  It's only called
when WT_PACKET is passed to the global tablet event hwnd message
queue.  WT_PACKET is only sent to that queue from XInput event
handlers that get registered when xinput is loaded.

In short GetCurrentPacket won't get called if xinput isn't loaded so I
don't believe it needs the xinput check.


As for the AttachEventQueue and WTInfoW checks:
I think we'd achieve the same end by having a loaded check in WTOpen
just like we do in WTInfo. The benefit would be that we'd let the app
know sooner that something was wrong.   As it is right now we try to
load but don't return 0 if we fail to load in
dll/wintab32/context.c:WTInfoT:

370     if (gLoaded == FALSE)
371          LoadTablet();
372

If we return 0 when loading fails in the dll/wintab32 WTInfo and
WTOpen entry points we save the trouble of running through a bunch of
function calls that are doomed to fail anyway. Even worse in most
places in the wintab code we don't check if the winex11 calls succeed
or fail.

This would require modifications to LoadTablet and its winex11 buddy
LoadTabletInfo so they could bubble up the failure.

Regards,
--John Klehm

winex11.drv: Ensure that wintab xinput_handle is not NULL.

[Peter Urbanec]

On 01/03/11 17:13, John Klehm wrote:
> Nice find on this code path being able to run without loading xinput.

It was as a result of a user sending me some crash logs. I never managed 
to reproduce this issue. I don't even have access to a tablet to test. 
My patch was pretty much a result of "NULL pointers seen in the wild -> 
better check for them" reaction.
> In short GetCurrentPacket won't get called if xinput isn't loaded so I
> don't believe it needs the xinput check.

Fair enough, I can remove that check and resend the patch.
> As for the AttachEventQueue and WTInfoW checks:
> I think we'd achieve the same end by having a loaded check in WTOpen
> just like we do in WTInfo. The benefit would be that we'd let the app
> know sooner that something was wrong.   As it is right now we try to
> load but don't return 0 if we fail to load in
> dll/wintab32/context.c:WTInfoT:
>
> 370     if (gLoaded == FALSE)
> 371          LoadTablet();
> 372
>
> If we return 0 when loading fails in the dll/wintab32 WTInfo and
> WTOpen entry points we save the trouble of running through a bunch of
> function calls that are doomed to fail anyway. Even worse in most
> places in the wintab code we don't check if the winex11 calls succeed
> or fail.
>
> This would require modifications to LoadTablet and its winex11 buddy
> LoadTabletInfo so they could bubble up the failure.

Sounds good. I'll get that done, but my testing is going to be limited 
due to the fact that I actually don't have a tablet to test with.