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

[Sketch] sysconf, pathconf, fpathconf #33

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

milseman
Copy link
Contributor

Just a sketchup of support for POSIX system variable configuration. Does not include the more complete, and platform-specific, sysctl.

NOTE: This is based on top of the cleanup and fixes in #32.

@milseman milseman requested a review from lorentey March 15, 2021 20:07
@milseman milseman marked this pull request as draft March 15, 2021 20:09
@milseman milseman mentioned this pull request Mar 22, 2021
@milseman
Copy link
Contributor Author

milseman commented Oct 3, 2021

@lorentey so I'm interested in landing these sketches. The API design question here is how best to surface global calls. Here we have a enum SystemConfig {} and a static get(_ name: Name) -> Int method for sysconf. An alternative would be some kind of shared namespace for all of these kinds of global calls, not just for configuration.

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

I like this, but I have many comments! It would be great if we could unify all these under a single API umbrella, but I have some doubts if that's the right direction.

///
/// The corresponding C function is `sysconf`.
@_alwaysEmitIntoClient
public static func get(_ name: Name) throws -> Int {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: naming suggestions to fit the API guidelines about non-mutating functions:

  public static func value(for name: Name) throws -> Int
  public static func systemConfiguration(named name: Name) throws -> Int

This is not a strong opinion -- I know I've flip-flopped about this, and I'll probably keep doing that. :-P

I think there would be value in using different base names for the various underlying calls (sysconf/pathconf/sysctl/what have you).

We could instead define a separate property for each supported option, but that won't work for sysctl or even pathconf. (We should do that anyway for the most high-profile options -- page size, core count etc.)

Copy link
Member

Choose a reason for hiding this comment

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

(How) does sysctl fit into this naming scheme?

*/

/// A namespace to access system variables
public enum SystemConfig {}
Copy link
Member

Choose a reason for hiding this comment

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

Config feels a bit unusual -- do we have a precedent for using it?

How about SystemConfiguration or just Configuration? (A quick search through the SDKs implies we prefer to spell this out.)

SystemInformation might be another option.

///
/// The corresponding C constant is `_SC_STREAM_MAX`
@_alwaysEmitIntoClient
public static var maxStreams: Self { Self(_SC_STREAM_MAX) }
Copy link
Member

Choose a reason for hiding this comment

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

Swift doesn't use FILE *, so I think this may be okay to exclude.

@_alwaysEmitIntoClient
public static var pageSize: Self { Self(_SC_PAGESIZE) }

/// The minimum maximum number of streams that a process may have open at
Copy link
Member

Choose a reason for hiding this comment

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

Typo: minimum maximum (here and below)

///
/// The corresponding C constant is `_SC_TZNAME_MAX`
@_alwaysEmitIntoClient
public static var maxTimezones: Self { Self(_SC_TZNAME_MAX) }
Copy link
Member

Choose a reason for hiding this comment

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

The man page is gibberish 🙈 -- this is the maximum length of a time zone name.

I don't think we need to expose this until/unless we expose C time zones, which seems unlikely right now.

///
/// The corresponding C constant is _SC_ARG_MAX) ``
@_alwaysEmitIntoClient
public static var maxArgumentBytes: Self { Self(_SC_ARG_MAX) }
Copy link
Member

Choose a reason for hiding this comment

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

It would make sense to delay introducing this until we start wrapping execve -- otherwise we risk defining this with inconsistent terminology. (E.g., I'm pretty sure we'd want to clarify that this is for command line arguments passed to a process, not e.g. function arguments.)

///
/// The corresponding C constant is _SC_IOV_MAX) ``
@_alwaysEmitIntoClient
public static var maxIOV: Self { Self(_SC_IOV_MAX) }
Copy link
Member

Choose a reason for hiding this comment

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

I think we should delay adding this until we start wrapping readv and friends.

///
/// The corresponding C constant is _SC_CLK_TCK) ``
@_alwaysEmitIntoClient
public static var clockTicks: Self { Self(_SC_CLK_TCK) }
Copy link
Member

Choose a reason for hiding this comment

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

I think we should delay adding this until we start wrapping the relevant stats syscalls.

///
/// The corresponding C function is `pathconf`.
@_alwaysEmitIntoClient
public static func get(_ name: PathName, for path: FilePath) throws -> Int {
Copy link
Member

Choose a reason for hiding this comment

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

Same naming nit as above. The function name ought to be a noun, and I think there would be value in differentiating pathconf from sysconf from sysctl.

///
/// The corresponding C constant is `Name`
@_alwaysEmitIntoClient
public static var processorsOnline: Self { Self(_SC_NPROCESSORS_ONLN) }
Copy link
Member

Choose a reason for hiding this comment

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

Does the configured vs online distinction make sense these days?

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