Thanks Eric, that makes things a lot cleaner, more resilient and, at a
guess, more portable (surely any OS with select will have O_NONBLOCK?)
I agree it's more a performance bug but, I think, if select returns a
spurious wakeup then the read should set EAGAIN (EWOULDBLOCK) - which
the Linux 3.13.0-65.105-generic kernel doesn't. (Not that this would
have helped the unpatched sox).
Cheers,
Tarim
Eric Wong wrote:
> Tarim <[email protected]> wrote:
>> I'm now thinking that this is actually a bug in the latest Ubuntu
>> kernel
>> 3.13.0-65.105-generic where select's FD_ISSET returns true even when
>> there is no data to read!
>
> This is a kernel bug from a performance standpoint, but the select
> manpage explicitly documents spurious wakeups as a possibility which
> userspace must be prepared for.
>
> The following patch should fix the problem:
>
> ------------------8<------------------
> Subject: [PATCH] use non-blocking stdin for interactive mode
>
> When accepting keyboard input, it is possible for select() to
> return a false-positive with spurious wakeups from inside the
> update_status callback.
>
> Using the FIONREAD ioctl in place of select is also a possibility,
> but may be less portable.
> ---
> Downloadable patch:
> http://80x24.org/spew/1443909787-27821-1-git-send-email-e%4080x24.org/raw
>
> In case upstream prefers a pull request:
>
> The following changes since commit
> 7e74b254b2a7c963be0bfce751fc5911fe681c12:
>
> Remove hepler script. It's mostly unmaintained, I don't know if anyone
> but me ever used it. In any case, those who want a custom Debian
> package should be capable of updating the debian/changelog entry on
> their own. (2015-02-26 22:48:40 -0500)
>
> are available in the git repository at:
>
> git://bogomips.org/sox nb-stdin
>
> for you to fetch changes up to 62a370a2a01cf1dcc0d5f9b9f49d1515706346cd:
>
> use non-blocking stdin for interactive mode (2015-10-03 22:04:50
> +0000)
>
> ----------------------------------------------------------------
> Eric Wong (1):
> use non-blocking stdin for interactive mode
>
> src/sox.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/sox.c b/src/sox.c
> index bab0f45..fdb7616 100644
> --- a/src/sox.c
> +++ b/src/sox.c
> @@ -1789,6 +1789,18 @@ static int process(void)
> tcsetattr(fileno(stdin), TCSANOW, &modified_termios);
> }
> #endif
> +#if defined(F_GETFL) && defined(F_SETFL) && defined(O_NONBLOCK)
> + if (interactive) {
> + int fd = fileno(stdin);
> + int flags = fcntl(fd, F_GETFL);
> + if (flags == -1) {
> + lsx_warn("error getting flags on stdin descriptor: %s",
> strerror(errno));
> + } else if (!(flags & O_NONBLOCK)) {
> + if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1)
> + lsx_warn("error setting non-blocking on stdin: %s",
> strerror(errno));
> + }
> + }
> +#endif
>
> signal(SIGTERM, sigint); /* Stop gracefully, as soon as we possibly
> can. */
> signal(SIGINT , sigint); /* Either skip current input or behave as
> SIGTERM. */
> --
> EW
>
------------------------------------------------------------------------------
_______________________________________________
SoX-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/sox-devel
