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

[Elias Vanderstuyft]

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?

> 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;

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"?
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)

>
>> b)  Weirdly, I had to add these commands that evaluates the DIEFFECT
>> pointers at the end of each if-case,
>>     otherwise the pointers seemed to point to other address or data on
>> that address was getting overwritten:
>>         TRACE("&rforce=0x%x\n", &rforce);
>>         TRACE("&cforce=0x%x\n", &cforce);
>>         TRACE("&pforce=0x%x\n", &pforce);
>>         TRACE("&cxyforce[0]=0x%x; &cxyforce[1]=0x%x\n",
>> &(cxyforce[0]), &(cxyforce[1]));
>>
>
> This is an out of scope problem. pforce, rforce, etc are local to the
> body of the if-blocks and fall out of scope when the if-blocks close.
> They should be moved to the top of the function.

Ah, interesting, thanks!
I'll move their declarations to the top of the function in the
corresponding patch that I will send.
To clarify for other people:
    Change joy.cpl/main.c:741 :
        dieffect.cAxes = 2;
        dieffect.rgdwAxes = axes;
        dieffect.rglDirection = direction;

        if (IsEqualGUID(&pdei->guid, &GUID_RampForce))
    to :
        dieffect.cAxes = 2;
        dieffect.rgdwAxes = axes;
        dieffect.rglDirection = direction;

        DIRAMPFORCE rforce;
        DICONSTANTFORCE cforce;
        DIPERIODIC pforce;
        DICONDITION cxyforce[2];

        if (IsEqualGUID(&pdei->guid, &GUID_RampForce))

>
> By the way, when you begin to send patches to wine-patches, please
> only send up to 2-4 at a time, in logical groups. That way they can be
> reasonably reviewed & resubmitted as necessary.

OK, no problem ;)


Elias