[RFC 1/11] Linux FF: Joystick control panel program

[Andrew Eikum]

On Fri, Mar 07, 2014 at 10:01:30PM +0100, Elias Vanderstuyft wrote:
> On Wed, Mar 5, 2014 at 3:49 PM, Andrew Eikum <aeikum at codeweavers.com> wrote:
> > On Tue, Mar 04, 2014 at 08:19:16PM +0100, Elias Vanderstuyft wrote:
> >> a)  When unloading conditional effects in joy.cpl, the program crashes:
> >>         When unloading the conditional effect array entries,
> >>         Wine tries to access an invalid (high) address because
> >>         dieffect.lpvTypeSpecificParams was not set at the time the
> >> effect was created.
> >>         So declare a valid 'standard' (maxed out) conditional effect.
> >>         For other unknown effects:
> >>             - The same can happen when CustomForce effects are available,
> >>                 but because Wine's FF dinput->linux translation does
> >> not support this yet,
> >>                 we simply print a FIXME to indicate a possible cause
> >> if a crash would happen.
> >>             - The same for other (unsupported) effects (this is
> >> probably not safe?),
> >>                 but then we print a WARN.
> >
> > For unhandled types, it's probably best to print a FIXME and return
> > DIENUM_CONTINUE, rather than continue to rely on uninitialized data.
> 
> Alright.
> Should I also write the extra if-test for the CustomForce type, or do
> I have to cover it with one if-test as an unhandled type?
> 

I'd just treat it as an unhandled type.

> > Additionally, our IDirectInputEffect implementations could check for
> > NULL lpvTypeSpecificParams and fail appropriately instead of crashing,
> > if this is what Windows does.
> 
> Well, I checked the SetParameters() function, and it does correctly
> check on NULL lpvTypeSpecificParams and fail appropriately.
> The problem seems again to be related with joy.cpl/main.c: there is no
> check performed on the result of IDirectInputDevice2_CreateEffect(),
> so change joy.cpl/main.c:784 :
>     hr = IDirectInputDevice2_CreateEffect(
>         joystick->device, &pdei->guid, &dieffect,
> &joystick->effects[joystick->cur_effect].effect, NULL);
> 
>     joystick->effects[joystick->cur_effect].params = dieffect;
>     joystick->effects[joystick->cur_effect].info = *pdei;
>     joystick->cur_effect += 1;
> 
>     return DIENUM_CONTINUE;
> to :
>     hr = IDirectInputDevice2_CreateEffect(
>         joystick->device, &pdei->guid, &dieffect,
> &joystick->effects[joystick->cur_effect].effect, NULL);
>     if (FAILED(hr))
>         return DIENUM_CONTINUE;
> 
>     joystick->effects[joystick->cur_effect].params = dieffect;
>     joystick->effects[joystick->cur_effect].info = *pdei;
>     joystick->cur_effect += 1;
> 
>     return DIENUM_CONTINUE;
> 

Sure.

> Before I knew the joy.cpl app was to be blamed, and was searching in
> the IDirectInputEffect implementions, I saw something that is not
> conform MSDN:
> JoystickAImpl_EnumEffects and JoystickWImpl_EnumEffects should check
> on DIENUM_CONTINUE to continue the enumeration, or DIENUM_STOP to stop
> it:
>     http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sdk.callback.dienumeffectscallback%28v=vs.85%29.aspx
> For example, in JoystickWImpl_EnumEffects(), each occurrence of
> "(*lpCallback)(&dei, pvRef);" should check whether the return value is
> DIENUM_CONTINUE or DIENUM_STOP. When the latter is the case, should we
> then first add a "goto" statement to jump to the last "if (xfd == -1)
> IDirectInputDevice8_Unacquire(iface);" statement, or directly return?
> And what should it return in that case, "DIERR_NOTINITIALIZED" (does
> stopping enumeration mean failure? that's not clear in MSDN), or
> "DI_OK"?

Probably the goto. I'm not sure about the return value, you would need
tests to be certain. Probably DI_OK is correct.

> And even worse, what if the return value of "(*lpCallback)(&dei,
> pvRef);" is not equal to DIENUM_CONTINUE or DIENUM_STOP? Should
> JoystickWImpl_EnumEffects() interpret it as the former or as the
> latter? (also not mentioned in MSDN)
> 

Again, you'd need tests to confirm.

>         dieffect.cAxes = 2;
>         dieffect.rgdwAxes = axes;
>         dieffect.rglDirection = direction;
> 
>         DIRAMPFORCE rforce;
>         DICONSTANTFORCE cforce;
>         DIPERIODIC pforce;
>         DICONDITION cxyforce[2];
> 
>         if (IsEqualGUID(&pdei->guid, &GUID_RampForce))
> 

Yes, but the declarations have to be at the very top of the function,
alongside all of the other declarations.

Andrew