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@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_sd... 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