On Mon, Mar 10, 2014 at 4:55 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Fri, Mar 07, 2014 at 10:23:18PM +0100, Elias Vanderstuyft wrote:
On Wed, Mar 5, 2014 at 3:54 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Tue, Mar 04, 2014 at 08:19:39PM +0100, Elias Vanderstuyft wrote:
=> This has been discussed (seehttp://www.mail-archive.com/linux-input@vger.kernel.org/msg08513.html), and the following is true: My opinion appeared to be correct, but for to be sure, I was recommended to apply the change in Wine as well.
Yes, we should also fix this in Wine to support "buggy" old kernels. It would be worth adding a short comment to the code explaining which kernel versions are affected by this bug.
At the moment, I did not receive any reaction yet on my kernel patch (although it was suggested by one of the kernel devs): http://www.spinics.net/lists/linux-input/msg29920.html So I don't know yet in what kernel the bug will be resolved. If I my Wine patches make it first to release, what should I write then? "pending as of <current date>"?
Oh, I didn't know it wasn't fixed yet. Maybe just briefly mention that the parameter can be incorrectly modified by the kernel. The Git commit date will point future developers to the correct timeframe.
OK. So I have the following new code: LinuxInputEffectImpl *This = impl_from_IDirectInputEffect(iface); int effectId_old;
TRACE("(this=%p)\n", This);
effectId_old = This->effect.id; if (ioctl(*(This->fd), EVIOCSFF, &This->effect) == -1) { /* Restore effect id since this can be incorrectly modified by the kernel in case of upload failure. */ This->effect.id = effectId_old;
b) Be more precise in returning errors.
Seems fine. If you can add tests (EINVAL seems easy to test, at least), that would be even better.
But that may be FF driver dependent, for example: - the (soon deprecated) ff-memless driver does not return an error when setting periodic effects' period to zero - the newer ff-memless-next driver does return an error when setting periodic effects' period to zero, and when setting other invalid parameters And I don't know whether Wine tests may rely on drivers? (normally, then you would also need to own a FF capable joystick) Unless you use a dummy-device, of course, but again, I don't think we want to include that in the Wine test tree, right?
However on the other hand, I do understand that this should be tested. What do you suggest?
To be clear, I meant testing dinput's behavior on Windows. If this behavior changes on Windows based on which device you're using, then you're correct, we can't really write sane tests for it.
I only have Logitech FF joysticks, so I can't really tell. I remember on Windows, the Logitech drivers seem to fail silently. But as this might be driver dependent, I don't know what the best thing would be (, I would think that throwing an error is most logical: e.g. application developers should be noticed whether passing a specific parameter is invalid (with DIERR_INVALIDPARAM))
c) The following in dinput/effect_linuxinput.c:336 : if (res != DI_OK) should be probably : if (FAILED(res)) for example if a device reports S_FALSE because it has already updated an identical effect.
The same for line 549: if (retval != DI_OK) should be then : if (FAILED(retval))Sure, seems fine. As an additional fix, you could demonstrate with tests that Download and Start can return S_FALSE under those circumstances on Windows, and fix the Wine behavior to match.
I'm new to Wine's test-framework, suppose I managed to create such test and successfully run it on Wine, how can I compile that test for Windows? (I don't have MS VS, and I prefer to not use it)
There's instructions for cross compiling Wine tests here: http://wiki.winehq.org/CompilingDLLsUsingMingw In short, after installing the correct cross-compiler, just run "make crosstest" in the DLL's tests directory.
Thanks, very useful!
(Maybe I should also mention that I don't have enough time for the moment to create tests.)
Many of your patches don't need tests, as they're obviously correct or are making corrections to Linux-specific behavior. There are some patches where you need tests to determine correct Windows behavior.
Alright.
You could create a bug in Bugzilla and attach your patches there, mentioning what tests need to be written. Then the patches won't be lost on the mailing list, and perhaps another developer could take the time to write the tests confirming your changes.
Thanks for the advice.
Elias