fix(local_opendir): clear stale errno before the EINTR check

[KuzinAndrey]

Proposed changes

The Ticket #3987 workaround (27de037) reopens the directory when the first readdir() returns NULL and errno == EINTR, to recover from an interrupted readdir() on CIFS shares with Linux >= 5.1.

The check reads a stale errno: readdir() does not set errno when the directory stream is exhausted (man 3 readdir: "errno is not changed"), and errno is a per-thread value that any previously interrupted syscall leaves behind. mc's event loop routinely survives EINTR from select() and read() when the subshell's SIGCHLD or the terminal's SIGWINCH arrives, and success paths such as open, fstat and opendir do not clear it either.

With an empty directory and a stale errno == EINTR the condition is true on every iteration -- no syscall in the loop body writes errno on the success path -- so local_opendir() spins forever: opendir, fstat, getdents, closedir, repeat. Observed on a FUSE mount: four FUSE round trips per iteration (~80k/s), ~27% stime in mc and ~34% in the FUSE daemon, until the directory becomes non-empty or the process exits. On a local filesystem the same loop saturates a core with plain syscalls.

Set errno = 0 before the readdir() call so that only an EINTR reported by that readdir() itself triggers the reopen.

Checklist

• I have referenced the issue(s) resolved by this PR (if any)
• I have signed-off my contribution with git commit --amend -s
• Lint and unit tests pass locally with my changes (make indent && make check)
• I have added tests that prove my fix is effective or that my feature works
• I have added the necessary documentation (if appropriate)

[ossilator]

looks reasonable.

is this code covered by autotests by any chance? then an extension would be in order.

i find the commit message somewhat excessive - it doesn't really matter how errno can become non-null outside the strictly controlled local scope; it's an obvious programming error.

[KuzinAndrey]

I am currently developing a FUSE filesystem for personal use, and I ran into an issue where Midnight Commander would hang when accessing an empty directory. An AI agent identified the problematic spot through static analysis and by examining the state of the hung processes. I want to apply a quick fix without altering the existing codebase unnecessarily. I’m not sure how to write unit tests for this, as it involves a complex combination of system calls and the launching of external processes.

The commit message provides a detailed explanation of the situation to ensure there is no ambiguity.

A quick Google search on using readdir reveals a common recommendation: reset errno to zero before making the call. The existing code failed to do this.

[ossilator]

A quick Google search on using readdir reveals a common recommendation: reset errno to zero before making the call. The existing code failed to do this.

yes, exactly. that's all that is to say about this. the long analysis how it actually happens is entirely irrelevant.

[zyv]

Could you please adjust the commit message to start like all other ones in our project:

https://github.com/MidnightCommander/mc/commits/master/

"Ticket #5156: vfs - clear stale errno before checking for EINTR"

I also think that the rest of the commit message can be removed - it carries no additional information. Instead, I'd add a comment before reset to reference this ticket - if needed one can always check it for background information.

Otherwise LGTM, thank you. Unfortunately, we don't have VFS test for local VFS / local_opendir. I would also very much like to see one, but I wouldn't make it a prerequisite for merging this fix.

[KuzinAndrey]

I don't have time right now to make immediate changes; the @ossilator is also flagging issues in the loop involving the rewinddir call, so the logic for this entire section might need rethinking. You can amend the commit if these fixes are important, or I can try to rework the algorithm entirely later on.

[zyv]

I can try to rework the algorithm entirely later on.

I'm happy to wait unless Andrew wants to take care of this immediately. Thank you.

[zyv]

I have got $100 worth of promotional cloud credits from Anthropic for free and decided to use them to review this PR. The initial review used about 3M tokens over 1 hour of wall-clock time, but the results are quite impressive.

The agents wrote a minimal FUSE-based file system to reliably reproduce the problem and check that it's gone, as well as analyze the code in terms of other possible issues, approaches to fixing those, and corresponding tradeoffs.

I find it entertaining that they also didn't like the comment and proposed a change along the lines that I've suggested, and also echoed @ossilator's standing preference for splitting commits, which I actually concur with on principle, but am more susceptible to accept less perfect splits if nobody's willing to put the work into it (including myself). Otherwise, they have

• found a similar issue in the editor and signal handlers
• suggested that it's actually trivial to write a regression / unit test
• pointed to a possible fix at a different level to decrease the penalty on FUSE-based FS
• noted that skipping rewinddir as suggested and implemented breaks on XNU / BSD
• surfaced a number of other pre-existing issues around the loop

I will push the fixes to this PR and update it with what is open, but what I'd rather not do, or not in this PR.

[ossilator]

i don't know whether this is genuine or just an artifact of gh's rendering, but the attribution of the AI commits seems fishy - they seem to have both the author set to claude and a co-authored-by claude trailer, which makes no sense.

[zyv]

I've pushed the updates. Here are the remaining open issues from Claude review and my evaluation:

#	Where	Finding	Evaluation
1	local_opendir() loop	No retry cap: a readdir() that fails with EINTR every time (e.g. a FUSE daemon answering every READDIR with EINTR) makes mc spin forever	Pre-existing, sounds more like a local DoS from an evil demon; I'd leave it as is
2	local_opendir() probe	A probe readdir() failing with anything but EINTR (EIO, EACCES, ESTALE, …) is swallowed and the directory is shown as empty	Pre-existing, risky behavioral change that could flood users with error messages (#4590)
3	local_opendir() probe	Probe + rewinddir() empties listings on FUSE filesystems that cannot seek directories (#4289, #4635) and fetches the first READDIR batch twice	Pre-existing, trade-off accepted in #4635, fixing needs a redesign, e.g. keeping the probed entry instead of rewinding

Findings along the same lines outside the scope:

#	Where	Finding	Evaluation
a	read_one_line() in src/editor/syntax.c	Same stale-errno bug class: continue on EINTR without clearerr() spins at EOF after one interrupted read	Real-world EINTR source uncertain (FUSE daemon answering FUSE_INTERRUPT on sync reads?); fix: clearerr (f); (and errno = 0;) before continue
b	SIGCHLD/SIGWINCH handlers	The handlers don't save and restore errno, so a signal between readdir() and the check can replace a real EINTR	A storm test hit it 3 times in 20M iterations: unlikely; fix: save and restore errno in each handler

@mc-worker, maybe you can consider fixing the editor problem since you care about the editor.

My promotional $100 are gone, and my conclusion is that this thing can do a lot of good if used appropriately, but it can also work as a mega-slopgun and maintainer DoS amplifier, which we are currently observing elsewhere.

I wonder if open source projects can get free access to this on a permanent basis. For me, it would make it possible to process PRs (at all) using my limited time. Or maybe I should apply as a Claude driver if such jobs exist.

I'm interested in your opinions.

[zyv]

i don't know whether this is genuine or just an artifact of gh's rendering, but the attribution of the AI commits seems fishy - they seem to have both the author set to claude and a co-authored-by claude trailer, which makes no sense.

This is apparently how Claude is set up to do it. It created the commits, so it sets itself as the author and I'm the committer, and it also sets the co-authored tag. This is also what I saw in other projects. I'm fine with that, but I could, of course, reset the author to myself. Is there any reason to bother, or is it fine as it is?

[ossilator]

This is apparently how Claude is set up to do it. [...] Is there any reason to bother, or is it fine as it is?

well, it's completely illogical. and nobody looks at the committer under normal circumstances, so your involvement is mostly obfuscated. the convention in projects that take this seriously is to put the "human overseer" in the author field.

I wonder if open source projects can get free access to this on a permanent basis.

i don't know about claude, but sufficiently highly rated (starred) projects can use copilot pro for free.

[zyv]

This is apparently how Claude is set up to do it. [...] Is there any reason to bother, or is it fine as it is?

well, it's completely illogical. and nobody looks at the committer under normal circumstances, so your involvement is mostly obfuscated. the convention in projects that take this seriously is to put the "human overseer" in the author field.

So what would you say about the current state of the PR? Is that okay?

I wonder if open source projects can get free access to this on a permanent basis.

i don't know about claude, but sufficiently highly rated (starred) projects can use copilot pro for free.

Oh, that's interesting. I'll have a look. Thank you.

[ossilator]

So what would you say about the current state of the PR? Is that okay?

i would --reset-author with my own identity on the AI commits, and trim the trailers a bit.

unsurprisingly, a lot of projects are currently considering this topic.
https://codereview.qt-project.org/c/meta/quips/+/742364 section "disclosure" seems relevant here.
the git project is also in the process of establishing a policy. it's ... controversial.

[zyv]

So what would you say about the current state of the PR? Is that okay?

i would --reset-author with my own identity on the AI commits, and trim the trailers a bit.

But that's what I did, didn't I? And what trailers are you talking about? All I left are sign-off tags that we use otherwise, and co-authored-by to identify the model:

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

[ossilator]

But that's what I did, didn't I?

ah, that's fine then. i can't see it on gh because it tries to be smart ...

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

this isn't a trailer that is semantically sane; the noreply part implies this.
the projects i'm aware of use Assisted-By, and leave off the email address.

[zyv]

/rebase

[zyv]

@mc-worker, what do you think about this one? I think I can still manage to merge it today before I leave if no changes are required from your side.

[zyv]

I have merged this one since it's been open for a while, and I think everything is addressed now. Hopefully, a real robustness improvement for both CIFS & FUSE users...