[mouse] bsd: Don't try to use SIGIO for input ABI >= 23

Signed-off-by: Adam Jackson <ajax@redhat.com>
---
 src/bsd_mouse.c | 2 ++
 1 file changed, 2 insertions(+)

Adam Jackson <ajax@redhat.com> writes:

> Signed-off-by: Adam Jackson <ajax@redhat.com>
> ---
>  src/bsd_mouse.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/src/bsd_mouse.c b/src/bsd_mouse.c
> index a2c8ec7..dc628d4 100644
> --- a/src/bsd_mouse.c
> +++ b/src/bsd_mouse.c
> @@ -546,8 +546,10 @@ usbMouseProc(DeviceIntPtr pPointer, int what)
>                  pInfo->fd = -1;
>              } else {
>                  xf86FlushInput(pInfo->fd);
> +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 23
>                  if (!xf86InstallSIGIOHandler (pInfo->fd, usbSigioReadInput,
>                                                pInfo))
> +#endif
>                      AddEnabledDevice(pInfo->fd);

I looked at this code and walked away -- why isn't this just using
xf86AddEnabledDevice?

On Tue, 2016-06-07 at 13:09 -0700, Keith Packard wrote:
> Adam Jackson <ajax@redhat.com> writes:
> 
> > Signed-off-by: Adam Jackson <ajax@redhat.com>
> > ---
> >  src/bsd_mouse.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/bsd_mouse.c b/src/bsd_mouse.c
> > index a2c8ec7..dc628d4 100644
> > --- a/src/bsd_mouse.c
> > +++ b/src/bsd_mouse.c
> > @@ -546,8 +546,10 @@ usbMouseProc(DeviceIntPtr pPointer, int what)
> >                  pInfo->fd = -1;
> >              } else {
> >                  xf86FlushInput(pInfo->fd);
> > +#if GET_ABI_MAJOR(ABI_XINPUT_VERSION) < 23
> >                  if (!xf86InstallSIGIOHandler (pInfo->fd, usbSigioReadInput,
> >                                                pInfo))
> > +#endif
> >                      AddEnabledDevice(pInfo->fd);
> 
> I looked at this code and walked away -- why isn't this just using
> xf86AddEnabledDevice?

No idea, which is a bit distressing. That code seems to have come in
between 6.7 and 7.0, and I can't find a git copy of that cvs history
anywhere (and the cvs services are long since shut down). Thankfully I
think I've found the old cvsroot on kemper, I'll get that imported.

That certainly would be a less icky solution though, will send an update.

- ajax

Adam Jackson <ajax@redhat.com> writes:

> That certainly would be a less icky solution though, will send an update.

Might be better to leave it for someone able to test the result?

On Tue, 2016-06-07 at 13:57 -0700, Keith Packard wrote:
> Adam Jackson <ajax@redhat.com> writes:
> 
> > That certainly would be a less icky solution though, will send an
> > update.
> 
> Might be better to leave it for someone able to test the result?

I suppose. The code is almost certainly broken as-is though, I can't
imagine trying to do both sigio and threaded input on the same fd would
go well (or be in any way reasonable to support).

That said we're not masking sigio off from the input thread, which is
probably also worth fixing.

- ajax

Adam Jackson <ajax@redhat.com> writes:

> That said we're not masking sigio off from the input thread, which is
> probably also worth fixing.

I hope this does what you mean:

InputThreadDoWork(void *arg)
{
    sigset_t set;

    /* Don't handle any signals on this thread */
    sigfillset(&set);
    pthread_sigmask(SIG_BLOCK, &set, NULL);

Adam Jackson <ajax@redhat.com> writes:

> I suppose. The code is almost certainly broken as-is though, I can't
> imagine trying to do both sigio and threaded input on the same fd would
> go well (or be in any way reasonable to support).

Looking usbSigioReadInput, xf86AddEnabledDevice and xf86SigioReadInput
shows that the existing code is just in-lining the old version of
xf86AddEnabledDevice.

I don't think we need to make this ABI dependent, we can just remove
usbSigioReadInput and unconditionally call xf86AddEnabledDevice. With
the old server code, it'd end up doing nothing different, and with the
new server code, it will end up getting threaded input as desired.

On Tue, 2016-06-07 at 16:23 -0700, Keith Packard wrote:
> Adam Jackson <ajax@redhat.com> writes:
> 
> > That said we're not masking sigio off from the input thread, which is
> > probably also worth fixing.
> 
> I hope this does what you mean:
> 
> InputThreadDoWork(void *arg)
> {
>     sigset_t set;
> 
>     /* Don't handle any signals on this thread */
>     sigfillset(&set);
>     pthread_sigmask(SIG_BLOCK, &set, NULL);

D'oh. Indeed, happy to be wrong there.

- ajax