[PATCH xserver 2/9] Remove SIGIO support for input [v3]

[Peter Hutterer]

On Mon, May 16, 2016 at 01:24:21AM -0500, Keith Packard wrote:
> Peter Hutterer <peter.hutterer at who-t.net> writes:
> 
> Thanks for taking a look at these. This one is less mechanical than I'd
> like, so some of the changes aren't obvious. It's definitely good to
> review them carefully.
> 
> >>  KdNotifyFd(int fd, int ready, void *data)
> >>  {
> >>      int i = (int) (intptr_t) data;
> >> -    OsBlockSIGIO();
> >>      (*kdInputFds[i].read)(fd, kdInputFds[i].closure);
> >> -    OsReleaseSIGIO();
> >
> > how comes you don't replace these with input_lock/input_unlock()? if there's
> > a specific reason it'd be better to have this in a follow-up patch, I'd
> > prefer this patch to be mostly mechanical.
> 
> Just that we won't need a lock here in the threaded input case; threaded
> input holds the input lock across all input processing. This could go in
> a separate patch, but of course with SIGIO removed, we wouldn't need
> locking anyways.
> 
> If I replaced them with input_lock/input_unlock, I'd just have to remove
> those calls when threaded input got added.

yeah, but again it's imo easier to have this as a plain replacement,
followed by a patch that says "Not needed here" after the sigio replacement.
better documentation for the archives because without any comments it's hard
to tell whether this is a mistake or not.

> 
> >>  static void
> >>  KdAddFd(int fd, int i)
> >>  {
> >> -    struct sigaction act;
> >> -    sigset_t set;
> >> -
> >> -    kdnFds++;
> >> -    fcntl(fd, F_SETOWN, getpid());
> >>      KdNonBlockFd(fd);
> >> -    AddEnabledDevice(fd);
> >
> >
> > it's not clear why you're dropping this bit.
> 
> (the fcntl should be self-evident)
> 
> The AddEnabledDevice call was not necessary, given the SetNotifyFd call
> just below. Was a harmless bug before threaded input as it just whacked
> the same mask that SetNotifyFd was going to whack. However, with
> threaded input, we don't want the main server select to see the input
> devices, so the call needs to be removed.

right, in that case it'd be better to remove the AddEnabledDevice() call in
a patch before this one and then just do the obvious replacements in this
commit?

> > same here.
> 
> Same reason.
> 
> >> -    {FLAG_USE_SIGIO, "UseSIGIO", OPTV_BOOLEAN,
> >> -     {0}, FALSE},
> >
> > not 100% here - if you take this out won't current configs with that option
> > now throw an error? should't we be more lenient here?
> 
> Good point. Leaving this in seems harmless enough. One wonders if this
> value has ever been used...
> 
> Do you want me to rearrange the other fixes above to make more sense? Or
> is it good enough to have explanation here?

long-term it's better to have separate patches here, it makes archeology
less mysterious :)

Cheers,
   Peter