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

Fix #same_file? by providing access to 64 bit inode numbers. #8355

Merged
merged 1 commit into from
Nov 8, 2019

Conversation

didactic-drunk
Copy link
Contributor

No description provided.

@didactic-drunk
Copy link
Contributor Author

Without this same_file? may return incorrect results.

@didactic-drunk didactic-drunk changed the title Provide access to 64 bit inode numbers. Fix #same_file? by providing access to 64 bit inode numbers. Oct 28, 2019
@straight-shoota
Copy link
Member

Could you elaborate on what this change does? Why is it necessary? Is it only necessary on macOS or do we need to port this to other platforms as well?

@didactic-drunk
Copy link
Contributor Author

MacOS has a 32 bit legacy and 64 bit API. The c include files #define fstat fstat64 to use the newer 64 bit API is used by default. The original fstat call and structure is kept for compatibility with older applications that haven't been recompiled.

I think Linux and *BSD went through the same process long ago. The Linux stat64 man page states:

Over time, increases in the size of the stat structure have led to three successive versions of stat(): sys_stat() (slot __NR_oldstat), sys_newstat() (slot __NR_stat), and sys_stat64() (new in kernel 2.4; slot __NR_stat64). The glibc stat() wrapper function hides these details from applications, invoking the most recent version of the system call provided by the kernel, and repacking the returned information if required for old binaries. Similar remarks apply for fstat() and lstat().

I assume whatever tool generated the bindings didn't take the #ifdef's or #define's in to account. This PR is a fix for it's errors.

MacOS is my main test platform. I'm not sure about the others but they appear to use the 32 bit API's.

Platforms using 32 bit inode numbers:

aarch64-linux-gnu/c/sys/types.cr:  alias InoT = ULong
aarch64-linux-musl/c/sys/types.cr:  alias InoT = ULong
arm-linux-gnueabihf/c/sys/types.cr:  alias InoT = ULong
i386-linux-gnu/c/sys/types.cr:  alias InoT = ULong
x86_64-freebsd/c/sys/types.cr:    alias InoT = UInt
x86_64-freebsd/c/sys/types.cr:    alias InoT = ULong
x86_64-linux-gnu/c/sys/types.cr:  alias InoT = ULong
x86_64-linux-musl/c/sys/types.cr:  alias InoT = ULong
x86_64-windows-msvc/c/sys/types.cr:  alias InoT = UShort

Platforms correctly using 64 bit inodes.

i386-linux-musl/c/sys/types.cr:  alias InoT = ULongLong
x86_64-openbsd/c/sys/types.cr:  alias InoT = ULongLong
x86_64-darwin/c/sys/types.cr:  alias InoT = UInt64 (*)

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Oct 29, 2019

Nope. ULong is target dependent, thus UInt64 on 64-bit targets, so the only targets that don't use UInt64 are arm-linux-gnueabihf, i386-linux-gnu, and windows, and it's not necessary for them to have stat64.

What I see, however, is that this PR actually broke stats on macOS :(

@ysbaddaden
Copy link
Contributor

You must also update struct dirent and its associated functions (opendir, readdir, rewinddir, closedir).

@didactic-drunk
Copy link
Contributor Author

Nope. ULong is target dependent, thus UInt64 on 64-bit targets, so the only targets that don't use UInt64 are arm-linux-gnueabihf, i386-linux-gnu, and windows, and it's not necessary for them to have stat64.

If ULong's are 64 bit then I guess most platforms don't need changes. I don't have them available for testing.

MacOS and FreeBSD still need changes. Proof:

info = File.info(__DIR__)
ino = info.@stat.@st_ino
p ino
p typeof(ino) => UInt32

The 32 bit and 64 stat structures have different layouts on MacOS. The current stat structure uses the 32 bit layout. This PR corrects it and changes the 32 bit fstat to the 64 bit fstat64.

Printing the inode number from crystal show a truncated value when compared to ls, stat, find, other command line programs and ruby -e 'p File.stat(".").ino' . The current code can't be correct.

This PR show values that match the other programs.

opendir corrections coming soon.

What I see, however, is that this PR actually broke stats on macOS :(

The specs work. Eyeballing the output compared to command line programs looks correct. What broke?

@ysbaddaden
Copy link
Contributor

ysbaddaden commented Oct 30, 2019

No, specs don't pass. test_darwin on CI has many failures related to files (Dir.glob, FileUtils, ...):

8897 examples, 31 failures, 16 errors, 8 pending

This can't be merged until this is fixed.

@didactic-drunk
Copy link
Contributor Author

@ysbaddaden The specs pass.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Thank you 👍

Note that x86_64-freebsd target is fine —InoT is ULong since FreeBSD 12+ only, it's handled properly.

@RX14 RX14 merged commit 5877333 into crystal-lang:master Nov 8, 2019
@RX14 RX14 added this to the 0.32.0 milestone Nov 8, 2019
@didactic-drunk didactic-drunk deleted the darwin_stat64 branch November 8, 2019 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants