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

[Peter Hutterer]

On Wed, May 11, 2016 at 01:54:51PM -0700, Keith Packard wrote:
> This removes all of the SIGIO handling support used for input
> throughout the X server, preparing the way for using threads for input
> handling instead.
> 
> Places calling OsBlockSIGIO and OsReleaseSIGIO are marked with calls
> to stub functions input_lock/input_unlock so that we don't lose this
> information.
> 
> xfree86 SIGIO support is reworked to use internal versions of
> OsBlockSIGIO and OsReleaseSIGIO.
> 
> v2: Don't change locking order (Peter Hutterer)
> v3: Comment weird && FALSE in xf86Helper.c
>     Leave errno save/restore in xf86ReadInput
>     Squash with stub adding patch (Peter Hutterer)
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
>  Xi/exevents.c                             |   4 +-
>  config/config.c                           |   4 +-
>  configure.ac                              |  14 ---
>  dix/devices.c                             |  10 +-
>  dix/ptrveloc.c                            |   4 +-
>  dix/touch.c                               |   8 +-
>  hw/dmx/input/dmxevents.c                  |  24 ++---
>  hw/kdrive/ephyr/ephyr.c                   |   4 +-
>  hw/kdrive/src/kinput.c                    |  74 ++-----------
>  hw/xfree86/common/xf86Config.c            |  24 -----
>  hw/xfree86/common/xf86Cursor.c            |   8 +-
>  hw/xfree86/common/xf86Events.c            |  29 +++---
>  hw/xfree86/common/xf86Helper.c            |   6 +-
>  hw/xfree86/common/xf86Init.c              |   6 +-
>  hw/xfree86/common/xf86PM.c                |   8 +-
>  hw/xfree86/common/xf86Xinput.c            |   8 +-
>  hw/xfree86/os-support/shared/sigio.c      |  57 +++++-----
>  hw/xfree86/os-support/shared/sigiostubs.c |  23 -----
>  hw/xfree86/os-support/xf86_OSproc.h       |   4 -
>  include/input.h                           |   9 ++
>  include/os.h                              |   6 --
>  include/xorg-config.h.in                  |   3 -
>  mi/mieq.c                                 |   6 +-
>  mi/mipointer.c                            |   4 +-
>  os/utils.c                                |  55 +---------
>  test/Makefile.am                          |   2 +-
>  test/os.c                                 | 166 ------------------------------
>  xkb/xkbActions.c                          |   4 +-
>  28 files changed, 113 insertions(+), 461 deletions(-)
>  delete mode 100644 test/os.c
> 

[...]

> diff --git a/hw/kdrive/src/kinput.c b/hw/kdrive/src/kinput.c
> index 52be7ad..b415a86 100644
> --- a/hw/kdrive/src/kinput.c
> +++ b/hw/kdrive/src/kinput.c
> @@ -108,38 +108,6 @@ extern const char *kdGlobalXkbLayout;
>  extern const char *kdGlobalXkbVariant;
>  extern const char *kdGlobalXkbOptions;
>  
> -static void
> -KdSigio(int sig)
> -{
> -    int i;
> -
> -    for (i = 0; i < kdNumInputFds; i++)
> -        (*kdInputFds[i].read) (kdInputFds[i].fd, kdInputFds[i].closure);
> -}
> -
> -#ifdef DEBUG_SIGIO
> -
> -void
> -KdAssertSigioBlocked(char *where)
> -{
> -    sigset_t set, old;
> -
> -    sigemptyset(&set);
> -    sigprocmask(SIG_BLOCK, &set, &old);
> -    if (!sigismember(&old, SIGIO)) {
> -        ErrorF("SIGIO not blocked at %s\n", where);
> -        KdBacktrace(0);
> -    }
> -}
> -
> -#else
> -
> -#define KdAssertSigioBlocked(s)
> -
> -#endif
> -
> -static int kdnFds;
> -
>  #ifdef FNONBLOCK
>  #define NOBLOCK FNONBLOCK
>  #else
> @@ -171,51 +139,25 @@ static void
>  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.

>  }
>  
>  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.


>      SetNotifyFd(fd, KdNotifyFd, X_NOTIFY_READ, (void *) (intptr_t) i);
> -    memset(&act, '\0', sizeof act);
> -    act.sa_handler = KdSigio;
> -    sigemptyset(&act.sa_mask);
> -    sigaddset(&act.sa_mask, SIGIO);
> -    sigaddset(&act.sa_mask, SIGALRM);
> -    sigaddset(&act.sa_mask, SIGVTALRM);
> -    sigaction(SIGIO, &act, 0);
> -    sigemptyset(&set);
> -    sigprocmask(SIG_SETMASK, &set, 0);
>  }
>  
>  static void
>  KdRemoveFd(int fd)
>  {
> -    struct sigaction act;
>      int flags;
>  
> -    kdnFds--;
> -    RemoveEnabledDevice(fd);

same here.

>      RemoveNotifyFd(fd);
>      flags = fcntl(fd, F_GETFL);
>      flags &= ~(FASYNC | NOBLOCK);
>      fcntl(fd, F_SETFL, flags);
> -    if (kdnFds == 0) {
> -        memset(&act, '\0', sizeof act);
> -        act.sa_handler = SIG_IGN;
> -        sigemptyset(&act.sa_mask);
> -        sigaction(SIGIO, &act, 0);
> -    }
>  }
>  
>  Bool

..

> diff --git a/hw/xfree86/common/xf86Config.c b/hw/xfree86/common/xf86Config.c
> index 0c067c0..e0e8e23 100644
> --- a/hw/xfree86/common/xf86Config.c
> +++ b/hw/xfree86/common/xf86Config.c
> @@ -697,7 +697,6 @@ typedef enum {
>      FLAG_AUTO_ENABLE_DEVICES,
>      FLAG_GLX_VISUALS,
>      FLAG_DRI2,
> -    FLAG_USE_SIGIO,
>      FLAG_AUTO_ADD_GPU,
>      FLAG_MAX_CLIENTS,
>  } FlagValues;
> @@ -755,8 +754,6 @@ static OptionInfoRec FlagOptions[] = {
>       {0}, FALSE},
>      {FLAG_DRI2, "DRI2", OPTV_BOOLEAN,
>       {0}, FALSE},
> -    {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?

Cheers,
   Peter