Skip to content

Dir::list_self doesn't work #34

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

Open
yshui opened this issue Sep 18, 2020 · 7 comments
Open

Dir::list_self doesn't work #34

yshui opened this issue Sep 18, 2020 · 7 comments

Comments

@yshui
Copy link

yshui commented Sep 18, 2020

File descriptors opened with O_PATH cannot be used in getdents64.

Dir::list_self always fail with a Bad file descriptor error

@Ekleog
Copy link
Contributor

Ekleog commented Dec 25, 2020

Worse than that: instead of failing with Bad file descriptor, it actually returns an endless stream of Bad file descriptor errors, which means attempts to debug it make it very complicated (especially when the compiler manages to detect it and optimize a .all(|r| matches!(r, Ok(...) if ...) into something that finishes… or I guess that's the reason why my code did not just always freeze on iterating on the list_self)

@tailhook
Copy link
Owner

@Ekleog, it's a normal way of how errors work on Iterator<Item=Result<_>>. If you do collect::<Result<...>> it finishes. If you do map or all you need to take some precautions to make it working with results.

Regarding original issue with list_self. We should probably ask @ksqsf how you expected it to be used? In the meantime you can use dir.list_dir('.') as was originally recommended.

@Ekleog
Copy link
Contributor

Ekleog commented Dec 25, 2020

I don't think that the infinite return of Err is how it should work, but I guess let's not discuss this here :)

What I was trying to say was more that apparently rustc was clever enough to optimize out code that was essentially .all(|r| matches!(r, Ok(foo if foo.file_name() == "stuff"))) into something that does not take infinitely long to execute.
Which, in turn, probably means that LLVM found out that there is no IO happening between two Err conditions, which allowed them to leverage UB to erase the whole block out. (someday… someday rustc will manage to not have LLVM UB on infinite loops without IO :))

The point here being: I'm relatively sure that if there is no IO between two Err conditions, it means that Dir::list_self is currently broken in a way that is probably not easily salvageable, and it may make sense to push a patch-release adding a comment to the documentation saying that this function is broken for now and should not be used.

Does what I'm saying make sense? Or are there maybe some circumstances where it actually currently works properly?

@ksqsf
Copy link
Contributor

ksqsf commented Dec 26, 2020

I'd like to add the background of list_self here. It's added in #19, and was meant to address a problem where /some/path is an existing folder, but a FUSE FS is mounted there as well. And the FUSE FS needs information about the underlying folder. (That's why . doesn't work in this case.) My code mainly targets macOS so I didn't pay much attention to Linux, essentially assuming both kernels have the same semantics, which seems to be false.

To my understanding, both getdents64 and O_PATH are Linux-specific, so I don't have a say on that. Sorry. Maybe we can add a note saying "list_self() only works on certain platforms (notably not on Linux), and the use of list_dir(".") is recommended as long as there's no special requirements."

edit: or, better, write a correct implementation of list_self for Linux with the desired semantics.

@Ekleog
Copy link
Contributor

Ekleog commented Dec 30, 2020

Hmm… does FUSE actually not return the underlying folder's listing when using list_dir(".")? I'd definitely expect it'd do so, as <underlying_folder_reference>/. should always be equivalent to <underlying_folder_reference>.

To clarify: with the code

use std::io::BufRead;
use openat::Dir;

fn list(d: &Dir) {
    println!("Contents:");
    for e in d.list_dir(".").unwrap() {
        println!("- {:?}", e.unwrap().file_name());
    }
}

fn main() {
    println!("Opening /tmp/foo");
    let dir = Dir::open("/tmp/foo").unwrap();
    list(&dir);
    println!("Please do `mv /tmp/foo /tmp/bar; mkdir /tmp/foo/{{,baz}}` then press enter");
    std::io::stdin().lock().lines().next();
    list(&dir);
}

I get (when starting with mkdir /tmp/foo/{,quux} and actually running the commands shown) the result

Opening /tmp/foo
Contents:
- "quux"
Please do `mv /tmp/foo /tmp/bar; mkdir /tmp/foo/{,baz}` then press enter

Contents:
- "quux"

Does this code behave differently on MacOS, or is there maybe some other behavior that's FUSE-specific and that cannot be emulated with mv?

@justin39
Copy link

I'm using opendir for roughly the same purpose as @ksqsf .

list_dir(".") seems to work as expected after the FUSE is mounted (on Linux), while list_self is definitely broken. Might be a good note to add to the docs, as mentioned!

@cehteh
Copy link
Contributor

cehteh commented Apr 21, 2021

I created a simple test that clones fd's with openat(orginal_fd, ".", O_DIRECTORY) :

https://gist.github.com/cehteh/ddd125e625a0fce2103ceddbfce190cf

This runs in a loop and prints the first 3 entries (attention, no error handling, it'll crash when there are no entries).

While running i made the following tests (Linux 5.11.15, Debian/bullseye):

  • mount --bind the dir in which it ran
  • mount --bind the subdir dir/src in which it ran
  • same again with sshfs

These tests shown that it consistently picked up the underlaying dir's entries and not from the mount on top. Next I will try to translate that to 'openat' (did that already, but something isn't right, needs more investigation).

Can anyone out there try to test/replicate this on other Operating Systems and report the outcomes?

cehteh added a commit to cehteh/openat that referenced this issue Apr 21, 2021
* With FdType/fd_type() one can determine the kind of an underlying file descriptor.
  Lite descriptors are implemented only in Linux (for now).
  When O_DIRECTORY is supported it uses fcntl() in favo over stat()

* clone_dirfd() tries to do 'the right thing' for duplicating FD's

* libc_ok() is a simple wraper for libc calls that return -1 on error, I will refactor the
  code in the next commits to make use of that more.

Please test this! So far it works for me on Linux.
cehteh added a commit to cehteh/openat that referenced this issue Apr 21, 2021
* With FdType/fd_type() one can determine the kind of an underlying file descriptor.
  Lite descriptors are implemented only in Linux (for now).
  When O_DIRECTORY is supported it uses fcntl() in favo over stat()

* clone_dirfd() tries to do 'the right thing' for duplicating FD's

* libc_ok() is a simple wraper for libc calls that return -1 on error, I will refactor the
  code in the next commits to make use of that more.

Please test this! So far it works for me on Linux.
cehteh added a commit to cehteh/openat that referenced this issue Apr 21, 2021
* With FdType/fd_type() one can determine the kind of an underlying file descriptor.
  Lite descriptors are implemented only in Linux (for now).
  When O_DIRECTORY is supported it uses fcntl() in favo over stat()

* clone_dirfd() tries to do 'the right thing' for duplicating FD's

* libc_ok() is a simple wraper for libc calls that return -1 on error, I will refactor the
  code in the next commits to make use of that more.

Please test this! So far it works for me on Linux.
cehteh added a commit to cehteh/openat that referenced this issue Apr 21, 2021
This up/downgrade cloning converts into normal/lite handles which was missing before.

I hope this fixes tailhook#34 finally.
cehteh added a commit to cehteh/openat that referenced this issue Apr 21, 2021
This up/downgrade cloning converts into normal/lite handles which was missing before.

I hope this fixes tailhook#34 finally.
@cehteh cehteh mentioned this issue Apr 30, 2021
zackw added a commit to zackw/openat that referenced this issue Dec 12, 2023
It calls fsync() on the file descriptor.  This is necessary with some
operating systems and/or file systems to ensure that changes to the
contents of the directory make it to persistent storage — for the same
reason it is sometimes necessary to use `std::fs::File::sync_{all,contents}`.

Note that this method will fail if used on a `Dir` opened with `O_PATH`.
(Same issue as `list_dir`; see tailhook#34.)

Closes tailhook#47.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants