[RFC PATCH] Handle hotplugged devices from xorg.conf

[Dan Nicholson]

On Thu, Dec 4, 2008 at 4:32 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> On Wed, Dec 03, 2008 at 11:49:16PM -0800, Dan Nicholson wrote:
>> On Wed, Dec 3, 2008 at 7:28 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
>> > On Wed, Dec 03, 2008 at 07:20:26AM -0800, Dan Nicholson wrote:
>> >> Hotplug hooks are added to the HAL core to allow the DDX to handle device
>> >> hotplugging. If a registered hook returns False, config/hal will continue
>> >> processing the event. This allows the DDX to examine the type of device
>> >> and deciding whether to handle it or not.
>> >
>> > I'm not quite sure if that approach really fixes the issue - namely to avoid
>> > duplicated device.
>> > Duplicate devices can only occur if we have devices in the xorg.conf and we're
>> > using HAL hotplugging. HAL will always provide a /dev/input/eventX path for
>> > the device.
>>
>> The crucial feature here (to me) is not the avoidance of duplicate
>> devices. That's just a nice side benefit. It's that you can continue
>> configuring devices in xorg.conf, which is the Xorg global
>> configuration store. As opposed to an fdi file.
>
> Oh, right. then i misunderstood the main intention. Anyway:
> Your solution (as I understand) is designed to configure specific devices. The
> HAL approach is particularly good at configuring classes of devices. e.g. all
> mice should be added with this driver, all touchpads with that driver, etc.
> Configuring classes of devices is especially handy if you don't know which
> devices will be available later which affects basically any new installation.

I agree that HAL handling classes of devices is great, and putting
devices in xorg.conf doesn't handle that case at all.

> The problem we have now though is that it splits the configuration, and with
> this patch even more so. As you said, "there is something to be said for
> having a global configuration". I think we either need a way in xorg.conf to
> classify groups of devices, or a way to ease particular device configuration
> in HAL. Trying to unify them will just cause more problems.

The people I was aiming at are the ones who have devices in xorg.conf right now.

"My options are being ignored!"
"Oh, you need to write an fdi file now."
"But it was already working in xorg.conf."

>> > As it stands now, I think your effort would be better spent on finding a way
>> > to modify hal setting through a proper tool so that people don't have to
>> > resort to adding their options to a fdi file. If we had such a tool, we could
>> > tell people to just stop using xorg.conf completely.
>>
>> I definitely think that tool needs to be made and would spend time
>> working on it. However, there is something to be said for having a
>> global configuration. If I setup my session to work right for the
>> trackpoint on my laptop, I don't want to then go and create that setup
>> for every other user, too.
>>
>> I mean, why not xorg.conf? That is Xorg's configuration file. I agree
>> that Xorg should be smart enough that I don't need a configuration
>> file in most cases. However, when I do, isn't it preferable to have it
>> be xorg.conf rather than for a seemingly orthogonal tool (HAL)?
>
> xorg.conf is a standalone configuration. HAL has dbus integration, so the
> bootup config could be read by other clients too. The first dbus
> implementation back in 2006 was just an API and had to be tied to HAL
> manually, the direct HAL connection was introduced later.

I'm not arguing against using HAL or dbus, and I think they make Xorg
much more useful. What I don't like is that the current implementation
breaks setups in bizarre (to the casual observer) ways. I mean it's
perfectly normal to configure output devices in xorg.conf. Until
recently, that was the case for input devices, too.

>> >> +    /* Is the InputDevice specified as a core device in the active layout? */
>> >> +    for (devs = xf86ConfigLayout.inputs; devs && *devs; devs++) {
>> >> +        IDevPtr idp = *devs;
>> >> +        if (strcmp(idev->identifier, idp->identifier) != 0)
>> >> +            continue;
>> >> +        if (idp->commonOptions &&
>> >> +            (xf86CheckBoolOption(idp->commonOptions, "CorePointer", FALSE) ||
>> >> +             xf86CheckBoolOption(idp->commonOptions, "CoreKeyboard", FALSE)))
>> >> +            return True;
>> >> +        if (idp->extraOptions &&
>> >> +            (xf86CheckBoolOption(idp->extraOptions, "CorePointer", FALSE) ||
>> >> +             xf86CheckBoolOption(idp->extraOptions, "CoreKeyboard", FALSE)))
>> >> +            return True;
>> >> +    }
>> >
>> > you really really want TRUE as default here.
>>
>> You mean as the default arg to CheckBoolOption? I want it to return
>> FALSE unless it's actually set to TRUE in the configuration. From my
>> understanding, passing TRUE would mean that I would get that back if
>> the Option wasn't set. Then I would incorrectly match devices that
>> weren't marked as CorePointer/Keyboard.
>
> the server's default behaviour is to assume all devices as core devices unless
> specified otherwise. Makes sense, as non-core devices wouldn't actually move
> the pointer and couldn't interact with non-XI apps.
> Really, the whole is-core and not-core isn't really that important anymore
> since the rework for input-hotplug.
>
> So you need to assume true by default unless specified otherwise in the
> config.

I think I'm misstating my intention with this function, and I think it
could be greatly simplified and rolled into the duplicate device
checking. What it should be called is
device_is_already_added_by_HandleConfigFile. :) But I think all the
Core business can be ignored if we just check if that identifier has
already been added. So, iterate inputInfo.device, comparing
idev->identifier and dev->name. If they match, then it was handled as
part of the layout or whatever and can be ignored. Does that make
sense? Does dev->name get set from idev->identifier?

>> >> +/* Add a hotplugged device from the configuration. It it's already been
>> >> + * added or is a core device, skip it. */
>> >> +static Bool
>> >> +add_hotplug_device(XF86ConfInputPtr conf, const char *udi)
>> >> +{
>> >> +    IDevPtr idev = NULL;
>> >> +    DeviceIntPtr pdev;
>> >> +    char *config_info = NULL;
>> >> +    Bool ret = False;
>> >> +    int rc;
>> >> +
>> >> +    idev = xcalloc(1, sizeof(*idev));
>> >> +    if (!idev)
>> >> +        goto unwind;
>> >> +
>> >> +    if (!configInput(idev, conf, X_CONFIG))
>> >> +        goto unwind;
>> >> +
>> >> +    config_info = xalloc(strlen(udi) + 6); /* "xf86:" and NULL */
>> >> +    if (!config_info) {
>> >> +        xf86Msg(X_ERROR, "xf86: Could not allocate name\n");
>> >> +        goto unwind;
>> >> +    }
>> >> +    sprintf(config_info, "xf86:%s", udi);
>> >> +
>> >> +    /* Skip duplicate devices. */
>> >> +    if (device_is_duplicate(config_info)) {
>> >> +        xf86Msg(X_WARNING, "xf86: Device %s already added. Ignoring.\n",
>> >> +                idev->identifier);
>> >> +        ret = True;
>> >> +        goto unwind;
>> >> +    }
>> >> +
>> >> +    /* Skip core devices when AllowEmptyInput is false. */
>> >> +    if (!xf86Info.allowEmptyInput && device_is_core(conf, idev)) {
>> >> +        xf86MsgVerb(X_INFO, 4, "xf86: Device %s listed as core. Ignoring.\n",
>> >> +                    idev->identifier);
>> >> +        ret = True;
>> >> +        goto unwind;
>> >> +    }
>> >
>> > this bit should - if correctly implemented - leave you with no input
>> > devices that control the visible sprite.
>>
>> I'm not sure I'm following. I'm basically just trying to emulate the
>> logic of checkCoreInputDevices. But it looks like I didn't do that
>> correctly since allowEmptyInput is only taken into account when an
>> InputDevice has Option Core*.
>
> As said before, we assume any device is a core device unless specified
> otherwise. In a default configuration, device_is_core should always return
> TRUE.

Right. I'll let checkCoreInputDevices worry about that and just skip
already added devices.

I'll resend the patch with these changes. If it's really not wanted,
then at least the archives will have it. :)

--
Dan