Hi Martijn,
Martijn van Duren wrote on Mon, Aug 09, 2021 at 02:15:42PM +0200:
> If we're stripping down fixio to a single function, why not
> inline the whole thing?
I deliberately tried to:
1. Keep patches that change behaviour as small as possible to make
review as simple as possible for people who fear they might be
affected but are not specifically interested in editline(3).
2. Make sure that reorg / cleanup patches do not change behaviour.
> Also, as far as my brain explains things to me, it could theoretically
> be possible that the signal handler kicks in right after we return a -1
> from read(2), making it possible to get something like an EIO and
> entering the sig switch statement. Assuming that a signal handler
> doesn't clobber our errno (which libedit doesn't do, but who knows what
> bad code is out there) we should probably only check the signal type
> when we know we have an EINTR.
Yes, that looks like a race condition bug that so far escaped my
attention. But it seems unrelated to what should happen with signals
in general, so can we keep fixing that bug separate?
> Finally, I don't understand why we only have a single retry on EAGAIN?
Not having *any* retries inside read_char() would look like a worthy
long-term goal to me, but i'm not yet completely sure that can be
reached. I would prefer to steer into the direction of fewer magic
retries rather than more of them. Either way, EAGAIN seems unrelated
to SIGINT, so i'd prefer to keep topics separate.
> If the application keeps resetting the FIONBIO in such a furious way
> that it happens more then once in a single call (no idea how that could
> happen) it might be an uphill battle, but we just burn away cpu cycles.
> It is not and should probably not be treated like something fatal.
Actually, if the application sets FIONBIO at all (even once), chances
are quite low in the first place that stuff works as inteded by the
author of the application. So i certainly wouldn't worry about an
application setting FIONBIO in a loop, we have significantly worse
problems than that. But again, that's a separate topic.
> Diff below does this. (minus 27 LoC)
I do not in general object to cleaning this code up, and getting rid
of read__fixio() indeed seems to be a long-term goal. But i hope
we will be able to remove all the (mostly broken) functionality
from read__fixio() in a step-by-step manner, and once the function
is empty, it will fade away without having to disturb the code in
read_char(). Either way - separate topic...
The most important short-term goal seems to fix sftp(1), including
the steps required to get that done in a clean way.
> The following ports use libedit (there might be a better way of
> finding them, but this works):
Hmm, thanks, that list feels useful.
> So if we decide which of our interpretations should take precedence
> it might be a good idea to put it into snaps for a while.
I don't think so in this case. Let's not over-use the feature of
putting stuff in snaps. I think that should be reserved for stuff
that is quite important and somewhat urgent and can't easily be
tested in a less disruptive way. But here, testing a program is
quite feasible once you know which program to test.
Besides, *if* this patch causes a change in behaviour of a port,
the most likely change is that a program that now ignores SIGINT
exits on SIGINT afterwards. That may be worth investigating and
making a decision on in each individual case, but it's not a
super-critical change in behaviour that might require testing
in snaps.
Yours,
Ingo
