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

linux/gnu: add utmp(5) default paths #1496

Closed
wants to merge 1 commit into from

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Sep 2, 2019

@rust-highfive
Copy link

r? @gnzlbg

(rust_highfive has picked a reviewer for you, use r? to override)

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 7, 2019

These should be extern statics without a default value.

@lucab
Copy link
Contributor Author

lucab commented Sep 7, 2019

@gnzlbg thanks for the reviews!

I have a couple of doubts when trying to apply your suggestion above (and I didn't spot any similar example in libc):

  • what's the actual type signature? I mean, is it a pointer or an actual array of bytes? If the latter, is the size harcoded based on the current observed value?
  • for the final linkage, where would the symbols actually be defined?

I tried defining a bare extern static, but then consumers fail to link with undefined reference to _PATH_UTMP.
I ran an objdump -Ttx on my local glibc, and indeed I cannot see the symbol in there.

That seems consistent to me with the fact that those are C pre-processor definitions and not actual constants, so they may not end up anywhere in the (public) ABI.

I also run latest bindgen on the header and its suggestion seems to agree on re-defining them:

pub const _PATH_UTMP: &'static [u8; 14usize] = b"/var/run/utmp\0";

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 7, 2019

What bindgen suggests sounds correct.

@lucab lucab force-pushed the ups/linux-gnu-paths branch from 729709a to cc95952 Compare September 7, 2019 10:18
@lucab
Copy link
Contributor Author

lucab commented Sep 7, 2019

Ack. I've updated the PR with that.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 7, 2019

There is an assertion triggering in ctest, which I believe is maybe due to ctest not supporting byte strings, e.g., you probably want to add a test for byte strings here: https://github.com/gnzlbg/ctest/blob/master/testcrate/src/t1.rs#L6 and then work up your way to adding support to ctest for these. Shouldn't be too hard since &'static str is already supported.

@joshtriplett
Copy link
Member

Because these are preprocessor definitions, they don't really have a precise type. There isn't a symbol being bound to here, and nothing will break if we give them a better type for Rust. As a result, I think it'd make more sense to define these as &'static str, and adapt ctest to match.

(Also, as a side note, whatever's depending on utmp/wtmp/btmp paths, please make it cope with those files not existing, as they aren't required to exist.)

I don't think we should add all of these, unless someone specifically needs them. Adding the ones people specifically ask for, that aren't historical relics, seems reasonable.

@bors
Copy link
Contributor

bors commented Dec 21, 2020

☔ The latest upstream changes (presumably #1999) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@JohnTitor
Copy link
Member

Closing as this is inactive for years and in favor of Josh's comment. Feel free to re-open if you really want them, we can re-consider it anytime.

@JohnTitor JohnTitor closed this Jun 28, 2021
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.

6 participants