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

[discussion] ErrorKind::FilesystemLoop from io_error_more #130188

Open
GrigorenkoPV opened this issue Sep 10, 2024 · 8 comments
Open

[discussion] ErrorKind::FilesystemLoop from io_error_more #130188

GrigorenkoPV opened this issue Sep 10, 2024 · 8 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@GrigorenkoPV
Copy link
Contributor

GrigorenkoPV commented Sep 10, 2024

@rustbot label C-discussion

Main tracking issue: #86442

Background

The io_error_more feature introduced 21 new variants into ErrorKind. They were FCP'd back in December 2022, but there appeared to be quite a lot of disagreement about 4 of the added variants, so the stabilization (#106375) got stalled for over twenty months. Thankfully, the 17 uncontroversial variants got stabilized in #128316, so now we just need to iron out a satisfactory design for the remaining 4 variants, and then they can be stabilized too.

In order to not block any of the remaining variants on each other and to not intertwine the discussions, I've created 4 separate issues, which summarize the concerns & suggestions voiced up until this point and can serve as a place for further discussion.

FilesystemLoop

Currently corresponds to ELOOP on Unix and nothing ERROR_CANT_RESOLVE_FILENAME on Windows. (#86442 (comment), #130207)

Current docs description:

Loop in the filesystem or IO subsystem; often, too many levels of symbolic links.

There was a loop (or excessively long chain) resolving a filesystem object or file IO object.

On Unix this is usually the result of a symbolic link loop; or, of exceeding the system-specific limit on the depth of symlink traversal.

Make it correspond to ERROR_CANT_RESOLVE_FILENAME on Windows

Done in #130207

Old description

for ELOOP, Windows appears to give winapi::shared::winerror::ERROR_CANT_RESOLVE_FILENAME in similar situations (e.g. symlink loops). Could we add that in, or perhaps generalise FileSystemLoop to the slightly more general case of being unable to resolve?

Originally posted by Robert Collins in #86442 (comment)

In #86442 (comment) Ian Jackson voices a concern that this might not be the only place where ERROR_CANT_RESOLVE_FILENAME appears.

Chris Denton in #86442 (comment) and Robert Collins in #86442 (comment) confirm that this is the only place where Windows currently gives ERROR_CANT_RESOLVE_FILENAME and that there is a good correspondence with Unix's ELOOP (when it comes to symlikns, see below for the other usages of ELOOP).

Ian Jackson agrees with them in #106375 (comment), but proposes this should be done separately from stabilization.

There seems to be a consensus regarding this point.

Bikshed the name: be about loops in general, drop "filesystem" from the name

Unix's ELOOP is not just for symlink loops (or too long symlink chains).

ELOOP itself isn't returned solely when loops are detected. Add to that list mount(2) returning ELOOP for move operations where the target is a child of the source - something that has absolutely nothing to do with symlinks, and execve returning ELOOP for exceeding recursion limits during recursive script execution (since Linux 3.8).

  • because OS errors are moving targets, we cannot assume Linux / BSD / others will not introduce a 5th or 6th meaning, and its clear to me at least that Linux doesn't treat ELOOP as a filesystem error but a more general error.

I suggest renaming it to LoopError, but document that it means ELOOP on Linux and ERROR_CANT_RESOLVE_FILENAME on Windows, and either describe what we know right now, or provide breadcrumbs for readers to catch up.

Originally posted by Robert Collins in #86442 (comment)

I have a mild preference for renaming FilesystemLoop to something that doesn't include Filesystem, for the same reason: OSes do use it for other errors. For instance, Linux also uses it for keyrings, BPF, network routing/filtering, vhost, and network bridges.

Originally posted by Josh Triplett in #106375 (comment)

I disagree with renaming FilesystemLoop.

It is true that Unix has a tendency to reuse errno values, so that any particular errno value can often mean a variety of things. Particularly, less-common (even, obscure) APIs and facilities (ab)use errno values. Attempting to represent all these obscure possibilities leads to descriptions and categorisations that are vague and overlapping. We generally haven't done that and I don't think we should start now. (All of this was discussed at length in the earlier conversations in the tracking issue.)

The APIs available in std will produce this error for filesystem operations, not obscure other purposes. I think calling it FilesystemLoop is sensible.

Originally posted by Ian Jackson in #106375 (comment)

Bikeshed the name: be about symlink resolution failure in general, stop mentioning loops

some system calls on Linux also use ELOOP to mean "ELOOP A loop exists in symbolic links encountered during resolution of the path argument, or O_NOFOLLOW was specified and the path argument names a symbolic link." so I think interpreting it as "symlink loop or similar symlink resolve error was encountered" might be an accurate description, although (bike-shedding!) I don't know if FilesystemLoop is an accurate name then, and not something like SymlinkResolutionFailed or such...

Originally posted by Alain Emilia Anna Zscheile in #86442 (comment)

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. C-discussion Category: Discussion or questions that doesn't represent real issues. labels Sep 10, 2024
@Skgland

This comment was marked as resolved.

@GrigorenkoPV

This comment was marked as resolved.

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Sep 10, 2024
@ChrisDenton
Copy link
Member

Given the previous discussion, I have a mild preference for renaming this to LinkNotResolved (or something like that) as the error does seem more generic than just loops, even on Unix. Though I do get the argument for using Unix error names even if they're not entirely accurate.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 11, 2024
…LENAME, r=ChrisDenton

Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`

cc rust-lang#86442

As summarized in rust-lang#130188, there seems to be a consensus that this should be done.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 11, 2024
…LENAME, r=ChrisDenton

Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`

cc rust-lang#86442

As summarized in rust-lang#130188, there seems to be a consensus that this should be done.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2024
Rollup merge of rust-lang#130207 - GrigorenkoPV:ERROR_CANT_RESOLVE_FILENAME, r=ChrisDenton

Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`

cc rust-lang#86442

As summarized in rust-lang#130188, there seems to be a consensus that this should be done.
@Amanieu
Copy link
Member

Amanieu commented Sep 17, 2024

We discussed this in the libs-api meeting. For now we're considering not stabilizing this until we get a concrete use case for why a user would want to match on FilesystemLoop (or whatever we rename it to) other then to print an error message (for which we already have a mechanism).

@Amanieu Amanieu removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 17, 2024
@dtolnay
Copy link
Member

dtolnay commented Sep 17, 2024

#86442 (comment) allegedly has a use case. I think it's this: #86442 (comment) "mocking APIs, testing error handling and various other things"

@lucacasonato

@dtolnay
Copy link
Member

dtolnay commented Sep 17, 2024

Would something like UnresolvableIndirection be appropriate? "Unresolvable" — it doesn't say that you are specifically dealing with a loop. Maybe the system hit some limit trying to determine whether there is a loop, or maybe there is definitely no loop but you used O_NOFOLLOW. "Indirection" — it doesn't say that you are specifically dealing with symlinks or a filesystem. Maybe it's a network route, or keyring. But it captures the gist of all the usages described above.

@GrigorenkoPV
Copy link
Contributor Author

#86442 (comment) allegedly has a use case. I think it's this: #86442 (comment) "mocking APIs, testing error handling and various other things"

From what I can tell, that guy's complaint was "it is possible to get this error from std, yet I cannot produce it myself", which would apply to any unstable ErrorKind and is not really a concrete use-case.

@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 10, 2024
@ijackson
Copy link
Contributor

ijackson commented Dec 9, 2024

Would something like UnresolvableIndirection be appropriate? "Unresolvable" — it doesn't say that you are specifically dealing with a loop. Maybe the system hit some limit trying to determine whether there is a loop, or maybe there is definitely no loop but you used O_NOFOLLOW. "Indirection" — it doesn't say that you are specifically dealing with symlinks or a filesystem. Maybe it's a network route, or keyring. But it captures the gist of all the usages described above.

I think this is a bad name. It's the "descriptions and categorisations that are vague and overlapping" that I argued against in #106375 (comment). As I wrote:

It is true that Unix has a tendency to reuse errno values, so that any particular errno value can often mean a variety of things. Particularly, less-common (even, obscure) APIs and facilities (ab)use errno values. Attempting to represent all these obscure possibilities leads to descriptions and categorisations that are vague and overlapping.

As I argue there, the fact that Unix (or indeed, some other operating system) sometimes uses an error code for a situation which is not covered by the central meaning, does not mean that the name should not reference the central meaning. Instead, exceptions like this ought to be dealt with in the documentation.

I woiuld argue that the central meaning for ELOOP is loops of filesystem symlinks.

There is also value in reusing terms from the conventional strerror of the corresponding errno value, since that means that when a Rust program encounters a error used in an unusual way, it will emit an error similar to that of a correspondingly careful C program.

The fact that ELOOP is sometimes used for excessive depth, without necessarily verifying an actual loop, or in obscure situations for loops of other kinds, doesn't mean that we shouldn't include "loop", "filesystem" and "(sym)link" in the name. An attempt to capture all the possibilities in the name is misguided: the only description that will definitely remain true is "something went wrong somewhere", and even attempts to capture current usage go quickly into the weeds and produce bland, nearly-useless, names like "unresolvable something". (Note that "indirection" isn't always true; AIUI you can sometimes get ELOOP in certain fd passing attempts, which doesn't seem to me to be indirection.)

PS thanks to @GrigorenkoPV and others for picking this up again after I got blocked and discouraged by the libs team member's process violation in 2022. I'm glad to see that most of the new kinds got merged and that these remaining four are also getting there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants