-
Notifications
You must be signed in to change notification settings - Fork 360
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
sysconf adding few more constants. #4053
Conversation
@rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! However, I think except for _SC_OPEN_MAX it makes little sense for Miri to support them. Unless you have real-world evidence that this is useful, I'd rather not add those.
src/shims/unix/foreign_items.rs
Outdated
// the machine's fd table does not start with a pre-allocated size but just the | ||
// standard streams so we can't use it here and the spec imposes | ||
// a minimum of `_POSIX_OPEN_MAX` (20). | ||
("_SC_OPEN_MAX", |this| Scalar::from_int(20, this.pointer_size())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
20 seems really really low here. Why would we report such a low limit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can set to 1024 (what linux returns) some other platforms return higher values. wdyt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it dynamic on Linux, determined by ulimit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1024 being the max here indeed. but then 20 is too low for you so what do you propose ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ulimit
for FDs can easily be much higher, like 64k.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on illumos it s 65536 ; on my debian it is only 1024 by default with unlimited but as root I can push it higher indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess 1024 is not an unreasonable value. The comment should make it clear that this is completely arbitrary and we can adjust if it that turns out to be helpful.
src/shims/unix/foreign_items.rs
Outdated
// a minimum of `_POSIX_OPEN_MAX` (20). | ||
("_SC_OPEN_MAX", |this| Scalar::from_int(20, this.pointer_size())), | ||
// The spec imposes a minimum of `_POSIX_ARG_MAX` (4096). | ||
("_SC_ARG_MAX", |this| Scalar::from_int(4096, this.pointer_size())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support exec anyway so I don't think there's any point in supporting this constant.
src/shims/unix/foreign_items.rs
Outdated
// The spec imposes a minimum of `_POSIX_ARG_MAX` (4096). | ||
("_SC_ARG_MAX", |this| Scalar::from_int(4096, this.pointer_size())), | ||
// The spec imposes a minimum of `_POSIX_CHILD_MAX` (25). | ||
("_SC_CHILD_MAX", |this| Scalar::from_int(25, this.pointer_size())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support process spawning so it seems pointless to support this.
src/shims/unix/foreign_items.rs
Outdated
// The spec imposes a minimum of `_POSIX_CHILD_MAX` (25). | ||
("_SC_CHILD_MAX", |this| Scalar::from_int(25, this.pointer_size())), | ||
// 16 here is the least common denominator (i.e. macos, solarish). | ||
("_SC_NGROUPS_MAX", |this| Scalar::from_int(16, this.pointer_size())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this is going to be helpful for any program Miri supports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that was just "anticipation" I planned try to use the value for this line instead of the hardcoded 1 to see if this help ; not urgent tough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net
is anyway not supported by Miri, so seems a bit premature to add that constant already.
919ec09
to
d547966
Compare
7205950
to
e713faa
Compare
e.g. _SC_NGROUPS_MAX can be useful for socket ancillary data.