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

readdir returns d_ino==-1 in offset==0 mode #583

Closed
mpartel opened this issue Jan 30, 2021 · 5 comments · Fixed by #584
Closed

readdir returns d_ino==-1 in offset==0 mode #583

mpartel opened this issue Jan 30, 2021 · 5 comments · Fixed by #584
Labels

Comments

@mpartel
Copy link
Contributor

mpartel commented Jan 30, 2021

(Background: @McBane87 and I are trying to upgrade bindfs to FUSE 3.)

Always passing 0 as the offset to the readdir filler function ("mode 1" in fuse.h) causes clients to always see d_ino == -1 when they readdir (test program).

  • The passthrough example exhibits this problem, but passthrough_fh and passthrough_ll don't.
  • use_ino is set.
  • It doesn't matter whether FUSE_FILL_DIR_PLUS is passed.

Possible fix

On this line, remove off && .

Then passthrough passes through inode numbers if I just change it to (always) pass FUSE_FILL_DIR_PLUS.

I don't know the code well enough to tell whether that change is safe. It doesn't break any tests at least.

@Nikratio
Copy link
Contributor

Thanks for the report!

Looks this check has been there ever since the first introduction of readdirplus support in 2014.

Looking at the code, I am not sure why there is a condition on off here. On the other hand, I also do not see how always passing 0 is expected to work. If you look further down, then setting off == 0 bypasses all sorts of buffer extension logic. I think the check on off is meant to check if this is the first call to the function, and calling the function repeatedly with off==0 means things will not work right.

@mpartel
Copy link
Contributor Author

mpartel commented Feb 1, 2021

off == 0 is documented here and this codepath seems to handle it correctly, by adding the entry to a linked list, which is later copied into the buffer. Also this is in agreement with the documentation that off should not be 0 even in the first call if the 2nd mode is desired.

The first if-else in fill_dir_plus seems entirely unrelated to the filling strategy. It doesn't even pass dh anywhere. So I'm pretty convinced now that the off check can be removed.

@Nikratio
Copy link
Contributor

Nikratio commented Feb 1, 2021

Makes sense - pull requests welcome :-). This would be a great opportunity to add a unit test for this mode of operation, so that it doesn't break again in the future.

@Nikratio Nikratio added the bug label Feb 1, 2021
Nikratio pushed a commit that referenced this issue Feb 3, 2021
- Test added for all passthrough examples.
- passthrough.c uses offset==0 mode. The others don't.
- passthrough.c changed to set FUSE_FILL_DIR_PLUS to make the test pass.
- This fixes #583.
@jpandre
Copy link
Contributor

jpandre commented Mar 4, 2021

I seem to have fallen in the same trap under the same conditions.
But IMHO the fix is only partial : the st_mode is not set, which causes the d_type not to be returned in readdir().

Moreover, the st_ino is not taken from the "struct stat" provided by the filler when use_ino is set, resulting of d_ino returned as -1.

The patch below fixes that, but the conditions might not be correct, maybe st_mode should be filled unconditionally.

--- a/lib/fuse.c 2021-03-02 21:40:51.112958271 +0100
+++ b/lib/fuse.c 2021-03-04 15:50:52.294095236 +0100
@@ -3576,6 +3576,8 @@
return 1;
}
}

  • } else if (statp && f->conf.use_ino) {
  •   e.attr = *statp;
    
    } else {
    e.attr.st_ino = FUSE_UNKNOWN_INO;
    if (!f->conf.use_ino && f->conf.readdir_ino) {

@jpandre
Copy link
Contributor

jpandre commented Mar 4, 2021

The same patch again with proper fencing. Sorry about it.

--- a/lib/fuse.c	2021-03-02 21:40:51.112958271 +0100
+++ b/lib/fuse.c	2021-03-04 15:50:52.294095236 +0100
@@ -3576,6 +3576,8 @@
 				return 1;
 			}
 		}
+	} else if (statp && f->conf.use_ino) {
+		e.attr = *statp;
 	} else {
 		e.attr.st_ino = FUSE_UNKNOWN_INO;
 		if (!f->conf.use_ino && f->conf.readdir_ino) {

nh2 pushed a commit to nh2/libfuse that referenced this issue Mar 30, 2021
…e#584)

- Test added for all passthrough examples.
- passthrough.c uses offset==0 mode. The others don't.
- passthrough.c changed to set FUSE_FILL_DIR_PLUS to make the test pass.
- This fixes libfuse#583.
amir73il pushed a commit to amir73il/libfuse that referenced this issue Jun 9, 2021
…e#584)

- Test added for all passthrough examples.
- passthrough.c uses offset==0 mode. The others don't.
- passthrough.c changed to set FUSE_FILL_DIR_PLUS to make the test pass.
- This fixes libfuse#583.

(cherry picked from commit 5012a05)
amir73il pushed a commit to amir73il/libfuse that referenced this issue Jun 15, 2021
…e#584)

- Test added for all passthrough examples.
- passthrough.c uses offset==0 mode. The others don't.
- passthrough.c changed to set FUSE_FILL_DIR_PLUS to make the test pass.
- This fixes libfuse#583.

(cherry picked from commit 5012a05)
amir73il pushed a commit to amir73il/libfuse that referenced this issue Jun 21, 2021
…e#584)

- Test added for all passthrough examples.
- passthrough.c uses offset==0 mode. The others don't.
- passthrough.c changed to set FUSE_FILL_DIR_PLUS to make the test pass.
- This fixes libfuse#583.

(cherry picked from commit 5012a05)
amir73il pushed a commit to amir73il/libfuse that referenced this issue Jun 28, 2021
…e#584)

- Test added for all passthrough examples.
- passthrough.c uses offset==0 mode. The others don't.
- passthrough.c changed to set FUSE_FILL_DIR_PLUS to make the test pass.
- This fixes libfuse#583.

(cherry picked from commit 5012a05)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants