Skip to content

Store inode in Entry, add accessor #39

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 1 commit into
base: master
Choose a base branch
from

Conversation

cehteh
Copy link
Contributor

@cehteh cehteh commented Apr 19, 2021

the d_ino field is mandatory in POSIX and clients (me) really need it when passing data
around. Keeping it in the Entry saves an extra stat()/metadata query.

@cehteh cehteh force-pushed the entry_metadata_feature branch from d50be64 to 03117b6 Compare April 19, 2021 08:26
@cehteh
Copy link
Contributor Author

cehteh commented Apr 19, 2021

The 2nd commit may be not worth the costs of one additional file-descriptor. It started just
as thought experiment to implement Entry::metadata() which seems to be futile.

Nevertheless the functionality to 'remeber' the orginal directory is missing this can be done
on appplication side by wrapping the DirIter. Or I am considering to refine this to add a
'fat' DirIter to openat which includes the origin Dir and has some convinience
functions. Modestly lightweight would be to keep an origin: Option

in DirIter and only
store it on request (at construction time). The cost here is the file descriptor, not the memory.

Eventually when the necessary rust features stabilize (or there is some way to implement it) I
would still like Entry::metadata() (and few more: Entry::readlink(),..) possibly most
of the Dir api can be done on Entry as well...

Copy link
Owner

@tailhook tailhook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i think storing a duplicate dir is too costly.

I think this could be done in application wrapper. And when using that, I would probably put Dir into Arc<Dir> instead and use that instead of duplicating file descriptor. Arc<Dir> is tremendously useful and I even considered to having it by default in this crate, but decided to be a bit more low-level and less opinionated. This change fits the same category.

src/list.rs Outdated
@@ -37,6 +38,10 @@ impl Entry {
pub fn simple_type(&self) -> Option<SimpleType> {
self.file_type
}
/// Returns the inode number of this entry
pub fn inode(&self) -> u64 {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be libc::ino_t I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deliberately chosen the types from statx() (see #38) these are overall better suited than
the legacy types (which is a pain to deal with when differing in signedness). Arguably it
would been nice if statx() would define sx_ino_t or similar, but they are only aliases so
there isn't overly much value in doing so. In anticipation of statx() it would be a bit
awkward to return types as in struct stat which are sometimes inferior and kindof legacy.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note statx is a linux-specific call. This library also works on macos and different bsd variants.

Other than that, using libc types is convenient if you want to use other libc functions on that types. Because otherwise it will work on some platform but have compilation errors on others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware that statx() is linux only. but the types it uses are well chosen to be a superset of what 'struct stat' provides. My plan was to provide 'accessor' functions that use these statx() types even when statx() isn't available.

I have some hopes that eventually other vendors pick this up and it becomes standardized. Anyway it could be changed to 'struct stat' compatibility. (also in #38)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a new version which uses ino_t, removed the origin experiment.
The later will need more work and a complete different approach.

@cehteh
Copy link
Contributor Author

cehteh commented Apr 19, 2021

Yes i think storing a duplicate dir is too costly.

I think this could be done in application wrapper. And when using that, I would probably put Dir into Arc<Dir> instead and use that instead of duplicating file descriptor. Arc<Dir> is tremendously useful and I even considered to having it by default in this crate, but decided to be a bit more low-level and less opinionated. This change fits the same category.

I am after low level (as long it doesn't compromise the 'rust' feel) as well. I had Arc in mind but turned that down.
I'll see and experiment :)

@cehteh cehteh force-pushed the entry_metadata_feature branch from 03117b6 to 6855598 Compare April 21, 2021 17:45
the d_ino field is mandatory in POSIX and clients (me) really need it when passing data
around. Keeping it in the Entry saves an extra stat()/metadata query.
@cehteh cehteh force-pushed the entry_metadata_feature branch from 6855598 to abef3ec Compare April 21, 2021 17:50
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