Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ^\ handling with readline, without breaking ^D handling with readline. #116

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

jpco
Copy link
Collaborator

@jpco jpco commented Sep 6, 2024

(Re-)fixes #111. The problem with the first fix attempt in #113 is that it changed the code that calls callreadline() from thinking a NULL return value always means EOF to thinking it never does. In fact, it sometimes does! So this version is a little more nuanced, only setting errno = EINTR in callreadline() when a signal has interrupted things.

I will be less hasty this time and not submit this myself, even though I think it's a fairly uncontroversial bug fix ("surely this time it'll work...")

As an aside, I kind of think /signame should actually just SIG_IGN the signal in the local process and re-set it to SIG_DFL in setsigdefaults(). Seems like the most straightforward thing, and no more racey than the other signal behaviors already being handled by setsigdefaults(). It also (seems to) match bash and tcsh on my machine, but interestingly shells seem pretty diverse in what they actually do on signals at the prompt.

@jpco jpco added the bug label Sep 6, 2024
@jpco
Copy link
Collaborator Author

jpco commented Sep 9, 2024

Incredible. Almost this exact same fix was posted on the rc mailing list just over 30 years ago: https://inbox.vuxu.org/rc-list/[email protected]/

@wryun
Copy link
Owner

wryun commented Sep 11, 2024

If I were you, I'd consider that the same fix is in rc an endorsement... reminds me of my earlier dreams: #1

@jpco
Copy link
Collaborator Author

jpco commented Sep 11, 2024

I've been running with this for the last few days and it seems to work just fine. Between that and the vote of confidence from 1994, I feel good about merging as-is.

@jpco jpco merged commit a7db5c5 into wryun:master Sep 11, 2024
1 check passed
@jpco jpco deleted the sigquit2 branch September 11, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

signals = /sigquit does not ignore sigquit like it should
2 participants