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

[Dan Nicholson]

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.

>
>> +        else
>> +            idev->commonOptions = classopts;
>> +    }
>> +
>> +    return Success;
>> +}
>> +
>>  /**
>>   * Create a new input device, activate and enable it.
>>   *
>> diff --git a/hw/xfree86/parser/Configint.h b/hw/xfree86/parser/Configint.h
>> index 03509b3..80b5f00 100644
>> --- a/hw/xfree86/parser/Configint.h
>> +++ b/hw/xfree86/parser/Configint.h
>> @@ -203,6 +203,8 @@ else\
>>  "This section must have only one of either %s line."
>>  #define UNDEFINED_INPUTDRIVER_MSG \
>>  "InputDevice section \"%s\" must have a Driver line."
>
> why is this a condition? some drivers share the same options so it'd make
> sense to be able to share settings across drivers. unless it's really hard
> to do so, I'd make the driver line optional too.

Yeah, already removed. I'd forgotten it was in there because the code
using it was gone.

>> +#define UNDEFINED_INPUTCLASSDRIVER_MSG \
>> +"InputClass section \"%s\" must have a Driver line."
>>  #define INVALID_GAMMA_MSG \
>>  "gamma correction value(s) expected\n either one value or three r/g/b values."
>>  #define GROUP_MSG \
>> diff --git a/hw/xfree86/parser/InputClass.c b/hw/xfree86/parser/InputClass.c
>> new file mode 100644
>> index 0000000..1c98160
>> --- /dev/null
>> +++ b/hw/xfree86/parser/InputClass.c
>> @@ -0,0 +1,232 @@
>> +/*
>> + * Copyright (c) 2009 Dan Nicholson
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> + * obtaining a copy of this software and associated documentation
>> + * files (the "Software"), to deal in the Software without
>> + * restriction, including without limitation the rights to use,
>> + * copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following
>> + * conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> + * included in all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
>> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
>> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +/* View/edit this file with tab stops set to 4 */
>
> isn't shiftwidth 4 in this file? tabstop doesn't matter since you don't use
> tabs. (One reason to maybe take this line out ;)

Sure. I'm pretty sure I just copied another file and then started editing.

>> +
>> +#ifdef HAVE_XORG_CONFIG_H
>> +#include <xorg-config.h>
>> +#endif
>> +
>> +#include "xf86Parser.h"
>> +#include "xf86tokens.h"
>> +#include "Configint.h"
>> +
>> +extern LexRec val;
>> +
>> +static
>> +xf86ConfigSymTabRec InputClassTab[] =
>> +{
>> +    {ENDSECTION, "endsection"},
>> +    {IDENTIFIER, "identifier"},
>> +    {OPTION, "option"},
>> +    {DRIVER, "driver"},
>> +    {MATCH_PRODUCT, "matchproduct"},
>> +    {MATCH_VENDOR, "matchvendor"},
>> +    {MATCH_DEVICE_PATH, "matchdevicepath"},
>> +    {MATCH_IS_KEYBOARD, "matchiskeyboard"},
>> +    {MATCH_IS_POINTER, "matchispointer"},
>> +    {MATCH_IS_JOYSTICK, "matchisjoystick"},
>> +    {MATCH_IS_TABLET, "matchistablet"},
>> +    {MATCH_IS_TOUCHPAD, "matchistouchpad"},
>> +    {MATCH_IS_TOUCHSCREEN, "matchistouchscreen"},
>> +    {-1, ""},
>> +};
>> +
>> +#define CLEANUP xf86freeInputClassList
>
> doesn't seem to be used anywhere

See hw/xfree86/parser/Configint.h. Probably not how I would have
designed things, but...

Thanks for the review.

--
Dan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: iclass-revamp.patch
Type: application/mbox
Size: 2583 bytes
Desc: not available
Url : http://lists.x.org/archives/xorg-devel/attachments/20091217/4a40fe67/attachment.bin