[PATCH 01/12] dbus-core: Move to hw/xfree86/common dir

[Hans de Goede]

Hi,

On 01/29/2014 01:05 AM, Peter Hutterer wrote:
> On Tue, Jan 28, 2014 at 10:12:30AM +0100, Hans de Goede wrote:
>>>>> 8d972e0c xf86Xinput: Modify API for server-managed fd support
>>>>>     I worry a little that drivers won't necessarily support this -
>>>>> would be nice to check by forcing them to define
>>>>> I_SUPPORT_SERVER_MANAGED_FDS or something; either that, or extend
>>>>> InputDriverRec with an equivalent flag which must be set.
>>>>
>>>> Interesting idea, I've investigating this on my todo list for Monday
>>>> (I'm not working tomorrow).
>>>
>>> how about adding the pre-opened fd into a separate field and provide a 
>>> xf86GetManagedFD() call that merely returns that field.
>>> that way the driver code comes essentially down to:
>>>
>>> pInfo->fd = xf86GetManagedFD(pInfo);
>>> if (pInfo->fd < 0)
>>>     pInfo->fd = open("path");
>>>
>>> and supporting this in the drivers is easy with a few ifdefs, and the server
>>> is simple as well for the non-systemd case.  
>>
>> I don't see how this helps with Daniel's worry about drivers not being ported,
>> the way I see it for non ported drivers we don't want to call systemd_logind_get_fd
>> at all. So we need to check if the driver is capable of dealing with server
>> managed fds before calling systemd_logind_get_fd, which means delaying the
>> systemd_logind_get_fd till we have figured out which driver to use, and having some
>> flag in the driver to indicate it can deal with managed fds.
>>
>> This seems like an almost orthogonal problem to the API for the driver getting
>> the fd to me.
> 
> but wouldn't a new call do exactly that? the input driver sequence is
> something like this:
> 
> int
> SomeDriverPreInit()
> {
>     const char *device = xf86SetStrOption(pInfo->options, "Device", NULL);
>     if (!device)
>         return BadValue;
> 
>     pInfo->fd = open(device, O_RDWR|O_NONBLOCK);
>     ...
> }
> 
> one of the side-effects you get by re-using pInfo->fd is that both the
> server and the driver may believe they're in charge of the fd. Which
> shouldn't matter, but could become an issue. e.g. evdev leaves the fd open
> after PreInit as an "optimization", so if suddenly something else closes it
> that could cause issues.
> 
> OTOH, if you add a specific call to the driver the code changes to something
> like:
> 
> int
> SomeDriverPreInit()
> {
>     if (!xf86GetManagedFd(pInfo, &pInfo->fd)) {
>         const char *device = xf86SetStrOption(pInfo->options, "Device", NULL);
>         if (!device)
>             return BadValue;
> 
>         pInfo->fd = open(device, O_RDWR|O_NONBLOCK);
>     }
>     ...
> }
> 
> and xf86GetManagedFd() does the systemd_logind_get_fd() call.

Ah, so you want xf86GetManagedFd() to do the actual systemd_logind_get_fd() call,
I did not understand that the first time around, I thought you just wanted to
have a pInfo->serverfd and xf86GetManagedFd() be a wrapper to get that.

The problem with this is that at this time we are already pretty far in the
initialization of things, and xf86GetManagedFd() may fail because the device
is paused (in other words Xorg is currently not the active vt, the user has
switched away. In this case we don't want to continue as if there are no
server managed fds, we want to delay the probe / init until the user
switch backs to the vt xorg is running on.

This is best / easily done from the hotplug code, rather then from inside
the drivers. We could do things the way you suggested, but this would
require xf86GetManagedFd() to have a special return value, or a return
by reference through an argument, for the paused status. And the drivers
init method would then also need a special return to indicate it wants to
do a delayed probe, and currently we don't have any standardization of
driver return values, so that would be a problem too...

All in all I think it is just better to just make sure all relevant drivers
are fixed to support managed fds asap after merging these changes, so well
in advance of any official release of 1.16 .

> This way, any non-ported drivers call the normal open and fail if they lack
> permissions. Ported drivers continue to work regardless of systemd-logind
> support.

Note that ported drivers will already work regardless of systemd-logind
support.

> And if for some reason you can't do the get_fd() call in xf86GetManagedFd()
> you do what you're doing now (i.e. in the options) but just close the
> systemd-logind fd after EnableDevice if the driver didn't call
> xf86GetManagedFd().

That would be possible, but will only work if a device can be opened multiple
times without side-effects. Not sure this is worth the trouble, but if you want
I can change things to work this way.

Regards,

Hans