[RFC 2/11] Linux FF: Linux effect status management

[Elias Vanderstuyft]

On Mon, Mar 10, 2014 at 4:55 PM, Andrew Eikum <aeikum at 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 at codeweavers.com> wrote:
>> > On Tue, Mar 04, 2014 at 08:19:39PM +0100, Elias Vanderstuyft wrote:
>> >>     => This has been discussed (see
>> >> http://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