[RFC PATCH] Handle hotplugged devices from xorg.conf

[Peter Hutterer]

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.

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.

> > 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.
 
> >> +    /* 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.

> >> +/* 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.

Cheers,
  Peter