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

File-locking API uses incorrect fnctl constants on at least sparc64 #57007

Closed
glaubitz opened this issue Dec 20, 2018 · 12 comments
Closed

File-locking API uses incorrect fnctl constants on at least sparc64 #57007

glaubitz opened this issue Dec 20, 2018 · 12 comments

Comments

@glaubitz
Copy link
Contributor

As described in #49773 (comment), the file-locking API uses incorrect fnctl constants on sparc64.

Compare the glibc headers on sparc64:

https://github.molgen.mpg.de/git-mirror/glibc/blob/master/sysdeps/unix/sysv/linux/sparc/bits/fcntl.h#L53

and what's in the rust sources:

https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_data_structures/flock.rs.html#45

Would there be a mechanism to sync the constants directly with the glibc sources, i.e. "using the headers"?

@jrtc27 @karcherm @psumbera @michaelwoerister @alexcrichton

@glaubitz glaubitz changed the title File-locking uses incorrect fnctl constants on at least sparc64 File-locking API uses incorrect fnctl constants on at least sparc64 Dec 20, 2018
@jonas-schievink
Copy link
Contributor

I guess the mechanism would be "use the libc crate"

@glaubitz
Copy link
Contributor Author

@jonas-schievink Indeed, I forgot the most obvious solution.

The constants are there and usable: https://github.com/rust-lang/libc/blob/master/src/unix/notbsd/linux/other/b64/sparc64.rs

@glaubitz
Copy link
Contributor Author

glaubitz commented Dec 20, 2018

I assume the correct code would be:

            pub const F_RDLCK: libc::c_short = libc::F_RDLCK;
            pub const F_WRLCK: libc::c_short = libc::F_WRLCK;
            pub const F_UNLCK: libc::c_short = libc::F_UNLCK;
            pub const F_SETLK: libc::c_int = libc::F_SETLK;
            pub const F_SETLKW: libc::c_int = libc::F_SETLKW;

@karcherm
Copy link
Contributor

Also don't forget https://github.com/rust-lang/libc/blob/master/src/unix/notbsd/linux/other/mod.rs containing struct flock. As rustc_data_structures is already using the libc crate in that block, I strongly recommend to avoid to redefine either the structure or the constants in flock.rs, at least for the linux case.

@karcherm
Copy link
Contributor

This will break, because we lose the dummy field l_sysid, but I recommend to solve this a different way, like defining a make_flock function that adds l_sysid = 0 for the BSD case.

@michaelwoerister
Copy link
Member

This will break, because we lose the dummy field l_sysid, but I recommend to solve this a different way, like defining a make_flock function that adds l_sysid = 0 for the BSD case.

Could this be fixed in the libc crate itself?

@jrtc27
Copy link

jrtc27 commented Dec 20, 2018

Note that mips32 (both o32 and n32; mips64 ie n64 is fine) is the odd-one-out in Linux and does have l_sysid: https://github.com/rust-lang/libc/blob/8565755356f23baf6b2df933ff734ce2f00c8d9b/src/unix/notbsd/linux/mips/mips32.rs#L254.

@michaelwoerister
Copy link
Member

That sounds like it should already work.

@michaelwoerister
Copy link
Member

The code in rustc_data_structures/flock.rs is very old, from when we couldn't use any external crates in the compiler itself. I agree that we should switch the implementation to just using libc.

@karcherm
Copy link
Contributor

And actually, it does not matter at all for us, because in the FreeBSD documentation, it says

The l_pid and l_sysid
fields are only used with F_GETLK to return the process ID of the process
holding a blocking lock and the system ID of the system that owns that
process.

So this field is an output field used by an operation not used at all here. If it's not a syntax error to not specify it, we can just ignore its existence.

@michaelwoerister
Copy link
Member

If it's not a syntax error to not specify it, we can just ignore its existence.

We can handle this via either #[cfg!(...)] or std::mem::zeroed(). It would be even better if the struct implemented Default but I don't think it does.

@alexcrichton
Copy link
Member

Would there be a mechanism to sync the constants directly with the glibc sources, i.e. "using the headers"?

@michaelwoerister is spot on that these were added to rustc way before we could depend on libc and crates.io crates. The libc crate is the source of truth for these constants, and the libc-test crate in that repository is this sync with system headers

glaubitz added a commit to glaubitz/rust that referenced this issue Jan 6, 2019
Since the values for the fcntl constants can vary from architecture
to architecture, it is better to use the values defined in the libc
crate instead of assigning literals in the flock code which would
make the assumption that all architectures use the same values.

Fixes rust-lang#57007
bors added a commit that referenced this issue Jan 6, 2019
flock: Use fcntl constants directly from libc crate on Unix targets

Since the values for the fcntl constants can vary from architecture
to architecture, it is better to use the values defined in the libc
crate instead of assigning literals in the flock code which would
make the assumption that all architectures use the same values.

Fixes #57007
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