[PATCH v3 3/3] xfree86: Introduce InputClass configuration

[Dan Nicholson]

On Thu, Dec 17, 2009 at 6:28 AM, Dan Nicholson <dbn.lists at gmail.com> wrote:
> On Wed, Dec 16, 2009 at 10:56 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
>> On Mon, Dec 14, 2009 at 09:09:02PM -0800, Dan Nicholson wrote:
>>> Currently Xorg uses hal's fdi files to decide what configuration options
>>> are applied to automatically added input devices. This is sub-optimal
>>> since it requires users to use a new and different configuration store
>>> than xorg.conf.
>>>
>>> The InputClass section attempts to provide a system similar to hal where
>>> configuration can be applied to all devices with certain attributes. For
>>> now, devices can be matched to:
>>>
>>> * A substring of the product name via a MatchProduct entry
>>> * A substring of the vendir name via a MatchVendor entry
>>> * A pathname pattern of the device file via a MatchDevicePath entry
>>> * A device type via boolean entries for MatchIsKeyboard, MatchIsPointer,
>>>   MatchIsJoystick, MatchIsTablet, MatchIsTouchpad and MatchIsTouchscreen
>>>
>>> See the INPUTCLASS section in xorg.conf(5) for more details.
>>>
>>> Signed-off-by: Dan Nicholson <dbn.lists at gmail.com>
>>
>> I like it. I like it a lot, and I'd love to see this in 1.8.
>> I've got a few comments on the patch, the basic principle is working but
>> there are still some issues with the details of the approach (for extra fun
>> I tested with an xorg.conf.d).
>
> Thanks!
>
>>> +
>>> +/*
>>> + * Merge in any InputClass configurations. Each InputClass section can
>>> + * add to the original device configuration as well as any previous
>>> + * InputClass sections.
>>> + */
>>> +static int
>>> +MergeInputClasses(IDevPtr idev, InputAttributes *attrs)
>>> +{
>>> +    XF86ConfInputClassPtr cl;
>>> +    XF86OptionPtr classopts;
>>> +
>>> +    for (cl = xf86configptr->conf_inputclass_lst; cl; cl = cl->list.next) {
>>> +        if (!InputClassMatches(cl, attrs))
>>> +            continue;
>>> +
>>> +        xf86Msg(X_CONFIG, "%s: Applying InputClass \"%s\"\n",
>>> +                idev->identifier, cl->identifier);
>>> +        if (cl->driver && !idev->driver) {
>>
>> i'm not sure about this. with the current HAL code there's a a bit of a
>> dependency chain with the patch set as it is here:
>> the hal code only calls NIDR for devices with the x11_driver set. this
>> driver value is copied into idev->driver before MergeInputClasses is called.
>> which means that MIC is only ever called for devices with the driver already
>> set, making the "Driver" option in the input classes useless.
>>
>> looking at the man page where it says 'the "inputdriver" module
>> from  the  final  Driver  entry will be enabled' I think you had the right
>> intentions, just the wrong condition here.
>> IMO, xorg.conf should always override HAL anyway to guarantee a smooth
>> transition. (note to possible commenters: xorg.conf devices take a different
>> path and aren't affected by this at all)
>
> Yeah, I probably should have brought this up before. I went back and
> forth trying to figure out how this should be handled for HAL and for
> other config backends (udev) that could be designed more correctly.
> The man page information is probably from earlier when I really did
> have "last match wins" logic.
>
> I was thinking this would give a smoother transition for people who
> have customized HAL setups right now. They could unwittingly have
> their settings reset by a xorg.conf.d file they have no idea about.
> But I think you're probably right that whatever is in xorg.conf.d
> should probably be preferred and hopefully distros will set things up
> right.
>
>> anyway, your driver selection also contrasts with "Each class can only
>> supplement settings from a previous class, so it is best to arrange the
>> sections  with the most generic matches last." seems like the options are
>> taken from the first, the driver from the last?
>> i think the replacing approach may be better instead of the supplementing
>> approach, it allows for very basic catchall rules that are then modified and
>> refined, similar to what we have now (evdev for all, synaptics overwrites
>> for some).
>
> The code currently works on "first match wins" for all options even if
> the documentation lies. It's not hard to change, and the logic is all
> in MergeInputClasses in hw/xfree86/common/xf86Xinput.c. You can change
> the sense of the merging just by swapping the arguments in
> xf86optionListMerge. You could also merge all the InputClass sections
> settings together in whatever sense you want and then apply the NIDR
> options at the end. This would act such that xorg.conf.d always
> trumps.
>
>> also, it's hard to run out of numbers for filenames so one can easily stack
>> to the end of a chain but it's harder to stack to the front of a chain.
>> i.e. adding a 9999-special-option.conf is easier than a <need smaller
>> zero>-special-option.conf.
>
> I think the only real reason I did it this way was to match the first
> priority behavior when grabbing an InputDevice for core. Maybe that's
> not a big deal, though. I've attached
>
>>> +            idev->driver = xstrdup(cl->driver);
>>> +            if (!idev->driver) {
>>> +                xf86Msg(X_ERROR, "Could not allocate memory while merging "
>>> +                        "InputClass configuration");
>>
>> nitpick: I prefer "Failed to do something" over "Could not something", it
>> seems more expressive and grep-able.
>>
>>> +                return BadAlloc;
>>> +            }
>>> +        }
>>> +
>>> +        classopts = xf86optionListDup(cl->option_lst);
>>> +        if (idev->commonOptions)
>>> +            idev->commonOptions = xf86optionListMerge(classopts,
>>> +                                                      idev->commonOptions);
>>
>> this should be the other way round, otherwise the class options are replaced
>> with the commonOptions which makes it suprisingly hard to set simple things
>> like the keyboard layout through the class matching (it's already set in
>> HAL). This goes hand in hand with the above comment.
>
> Yep, see above comments. I've attached a patch on top of this one that
> makes the behavior "later .conf matches take precedence; all .conf
> matches override NIDR". See what you think about it.

Now I remember the real reason why I had this behavior. xorg.conf is
parsed before xorg.conf.d, and we presumably want entries there to
take precedence. I think the order of precedence should be:

xorg.conf
xorg.conf.d reverse alphasort order
NIDR

There are some ways we can hack this. In the xorg.conf.d patch, we
could reverse the alphasort sorting in scandir so that the files get
parsed backwards. Then looping over entries in xf86configptr would
give the desired effect of xorg.conf followed by the last file in
xorg.conf.d.

--
Dan