On Mon, 2021-08-09 at 15:17 +0200, Ingo Schwarze wrote:
> 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
>
Your reasoning makes sense to me.
Assuming you're confident enough with the applications linked to
libedit: OK martijn@
