Skip to content

Add/Refactor Dir::list() -> DirIter ctor #40

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
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

cehteh
Copy link
Contributor

@cehteh cehteh commented Apr 19, 2021

dir.list() consumes the dir and creates a DirIter. The underlying 'dup()' operation now
becomes explicit, one may need to call try_clone() first.

the list_self() and list_dir() using this now but are merely convinience wrappers and may (or
may not) deprecated in future.

Rationale: this simplifies the code a bit (list::open_dir() removed) and is closer to the low
level semantics.

dir.list() consumes the dir and creates a DirIter. The underlying 'dup()' operation now
becomes explicit, one may need to call try_clone() first.

the list_self() and list_dir() using this now but are merely convinience wrappers and may (or
may not) deprecated in future.

Rationale: this simplifies the code a bit (list::open_dir() removed) and is closer to the low
level semantics.
@cehteh cehteh marked this pull request as draft April 19, 2021 21:22
* Forgot to forget the Dir handle, thus it would been dropped.
* On linux O_PATH was passed when Dir descriptors where opened. This is generally a good thing
  but broke the refactored list(). This also shown that O_PATH has different enough semantics
  to be problematic vs. opening handles without (as on other OS'es). Changed this to
  Dir::open() and Dir::sub_dir() default to opening without O_PATH (but O_DIRECTORY see
  remarks).
* added Dir::open_path() and Dir::sub_dir_path() with O_PATH (urgh, better idea
  for naming? open_protected() or something like that?) which offer the O_PATH feature on linux.
@cehteh cehteh marked this pull request as ready for review April 20, 2021 00:29
@cehteh cehteh marked this pull request as draft April 20, 2021 05:46
@cehteh
Copy link
Contributor Author

cehteh commented Apr 20, 2021

There is a bug in the current codebase which blocks progress here:

try_clone() calls dup(), but dup()'ed descriptors share the seek offset and internal state, thus these clones can't act independently as iterators. This bug should be already prevalent without the commits fromt this pull request when one clones a Dir multiple times. But the list() introduced here made this apparent.

I am searching for a solution to make a 'real' clone of a fd a la:
'''
let dir = open("name", O_DIRECTORY);
let clone = openat(dir, ".", O_DIRECTORY);
'''
which will result in an independent descriptor (at some cost, but should be more correct).
For linux it can be optimized by dup() when O_PATH was set, (determined by fcntl()). But O_PATH fd's are not iterateable (see #34 which will be fixed as Bonus).

Further note: maybe O_SEARCH will be of help or may be an optional flag.

@tailhook
Copy link
Owner

That's interesting. Note also that openat(".") sometimes opens a different directory (if there is a mountpoint on top of the current one). See #34

@cehteh
Copy link
Contributor Author

cehteh commented Apr 20, 2021

That's interesting. Note also that openat(".") sometimes opens a different directory (if there is a mountpoint on top of the current one). See #34

Yes i will investigate this more closely, so far i consider that as experiment. While i had the impression/hopes that handles obtained openat will not shadowed by mount. Will see.

'Lite' file descriptors are possibly a better naming of what O_PATH does. This introduces then and
implements them portable. On systems which do not support O_PATH normal file descriptors are
used.
* 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 2 commits April 21, 2021 16:00
This simplifies the code a bit, in 'release' the same output is generated.
debug builds may not inline the libc_ok() and be slightly larger.
This up/downgrade cloning converts into normal/lite handles which was missing before.

I hope this fixes tailhook#34 finally.
@cehteh cehteh marked this pull request as ready for review April 21, 2021 15:01
@cehteh
Copy link
Contributor Author

cehteh commented Apr 22, 2021

Notice: I take back the idea of deprecate list_self() and list_dir() functions.
Rationale: Eventually O_SEARCH may be use for the descriptors cloned there which gives the kernel a chance of optimizing the iteration.

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

Successfully merging this pull request may close these issues.

2 participants