[RFC] Add Sysctl interface support for OSX/Linux/FreeBSD by elthariel · Pull Request #4352 · crystal-lang/crystal

elthariel · 2017-04-29T18:36:40Z

Hi,

This is an attempt at defining an interface to the sysctl API of the various UNIXes. OpenBSD is not yet supported as they don't have an easy way to get a sysctl by name (that I'm aware of). We could implement it by spawning a sysctl process or define a static map somewhere.

The underlying goal of this is to have a way to query the number of CPUs on OSX to provide a better default for the number of default threads on the multithread support branch(es). In addition to that we could use it for System.hostname, and I think this would be useful for many folks out there.

This has only been tested on OSX yet. I'll soon test it on Linux but I'll need help for FreeBSD. Any comment on this would be greatly appreciated, especially regarding general usefulness, design and method naming.

Unicornly yours,
Lta.

Signed-off-by: Julien 'Lta' BALLET contact@lta.io

Signed-off-by: Julien 'Lta' BALLET <contact@lta.io>

Sija · 2017-04-29T18:42:53Z

@@ -0,0 +1,87 @@
+lib LibC


This lib declaration IMO should be extracted to a separate file for each architecture (src/lib_c/<arch>/c/sysctl.cr).

I thought about it, but it would be to a lot of duplication, as the implementation would be the same for osx/freebsd and all the linux variants.
Also it was implemented this way in src/process/executable_path.cr

Should I still move it there ?

Sorry for the misinformation! as this is in lib_c you don't need the skip macro.

Shall I move this in the arch-dependant folders ?

Yes. The bindings in those folders should actually be automatically generated from the .h files on each platform, but I never got the generator to work. @ysbaddaden?

That's fun. I hacked something similar for OpenGL

Anyway, the variants of sysctl (sysctlbyname) I'm using isn't really POSIX compliant. Plus this syscall is marked as deprecated on Linux and is not available on linux libc. It doesn't look like it's going to play nicely with that script ?

I second that anything LibC should be in the arch specific under src/lib_c with file namings following the actual C headers; thus way we may be capable in the future to generate these on-the-flt.

We prefer POSIX, but don't enforce it, if it requires some specific but public API for a platform, or the function is so common across platforms that it can be relied on, you can use it.

That being said, if something is deprecated please don't use it; it will likely break some day.

@ysbaddaden, The syscall is deprecated on Linux but it's not available through the libc anymore, so no risks there. So your suggestion is to write those by hands until it is supported by the generator, am I right ?

Sija · 2017-04-29T18:43:33Z

+      path = name_to_path name
+      read_proc_sys(path)
+    {% else %}
+      raise NotImplemented("Sysctl", "is not supported on this platform")


Missing .new.

Sija · 2017-04-29T18:43:43Z

+      path = name_to_path name
+      read_proc_sys(path).to_i32
+    {% else %}
+      raise NotImplemented("Sysctl", "is not supported on this platform")


Missing .new.

Sija · 2017-04-29T18:45:01Z

+#
+class NotImplemented < Exception
+  def initialize(what : String, reason = "is not implemented")
+    super [what, reason].join(" ")


Use Tuple instead of an Array: {what, reason}.join(' ')

Sija · 2017-04-29T18:47:10Z

+      if File.readable?(path)
+        File.read(path)
+      else
+        raise Errno.new("Unable to read #{path}", Errno::ENOENT)


Shouldn't this be just Exception?

We might use the standard exception here but it would be inconsistent with the other system's implementation where we probably want to libc errno to be reported to the user.

Sija · 2017-04-29T18:47:16Z

+      if res == 0
+        String.new(value, value_len.value - 1, 1)
+      else
+        raise Errno.new("Unable to fetch sysctl value #{name}")


Shouldn't this be just Exception?

sysctl sets the errno in case of error, so I think we should propagate it.

As an aside: I think Errno is terrible design since it exposes platform-specifics at a high level, and on a more practical level you can't rescue only a single errno without using if inside rescue, however i'm too lazy to make an RFC

Just copy this/this and you've got an RFC 😜

IMHO, one of the point of Crystal is to allow system programming as well, which requires exposing platform specific details. That being said, I understand this should be allowed, not forced.

Sija · 2017-04-29T18:49:27Z

+  # Sysctl.get_int("hw.ncpu") # => 8
+  # ```
+  #
+  def get_i32(name : String) : Int32


I'd drop get_ prefix for consistency with Ruby/Crystal API expectations (namely no accessor - get_, set_, is_ - prefixes).

What would you name setters? I don't see a good alternative here.

makes perfect sense

@RX14

# getter def i32(name : String) : Int32; end # setter def i32(name : String, value : Int32) : Void; end

Perfetto!

@Sija Is there any precedence for that naming in the stdlib? I think it's terrible.

I've seen API like that in kemal-session shard for instance.

I've seen and done that in Ruby (and python fwiw) as well

Sija · 2017-04-29T18:49:33Z

+  # Sysctl.get_str("kernel.random.boot_id", 42) # => "1c629298-a60b-48df-bba1-9ea77f6b37ba"
+  # ```
+  #
+  def get_str(name : String, max_size : Int32) : String


RX14 · 2017-04-29T19:18:23Z

+# on a certain system or platform
+#
+# ```
+# Sysctl.get_int("net.ipv4.forward") # Sysctl doesn't exist on Windows


Typically we use # raises ExceptionType (message) when documenting raising in examples (ref)

RX14 · 2017-04-29T19:19:48Z

@@ -0,0 +1,87 @@
+lib LibC


RX14 · 2017-04-29T19:22:57Z

+  # Sysctl.get_int("hw.ncpu") # => 8
+  # ```
+  #
+  def get_i32(name : String) : Int32


What would you name setters? I don't see a good alternative here.

elthariel · 2017-04-29T19:51:14Z

Thanks to both of you for the quick and insightful review

Sija · 2017-04-29T19:52:27Z

+      {% elsif flag?(:linux) %}
+        ["net.ipv4.ip_forward", "kernel.pty.max"]
+      {% else %}
+        []


Should be [] of String (see failed travis build).

Signed-off-by: Julien 'Lta' BALLET <contact@lta.io>

ysbaddaden

I'm not sure this fits into the stdlib, mostly because this is platform specific. We may eventually make a few platform specific syscalls to extract some values (like the number of CPUs), but that would be internal to the corelib, not exposed: getting the number of available CPU on any platform is a great abstraction, raw access to sysctl isn't.

I think this would fit better as a Shard.

ysbaddaden · 2017-05-01T18:55:21Z

+  def initialize(what : String, reason = "is not implemented")
+    super {what, reason}.join(" ")
+  end
+end


No, this should fail to compile, not fail at runtime. One solution is to not expose the Sysctl type on unsupported platforms.

Shall we keep the NotImplemented exception ? (like ruby & python for example), should I move it to a separate PR ? or should I get rid of it ?

No. It may be meaningful in interpreted languages, not in a compiled language: not implemented => compilation failure.

Duh. Now you're pointing at it...
Would only be useful with dlopen & friends. I'll get rid of it tomorrow

elthariel · 2017-05-02T11:14:28Z

@ysbaddaden, Even if it Linux deprecated it, sysctlis still part of POSIX and libc and generally widely available on Unix.
Also, I'm not sure I understand Crystal goal here. By providing things like pointers and std access to libc, it kind of positions itself to some extent as a system language. And if you're a system language, it seems to me that you want access to sysctl and ioctl, no ? It also doesn't prevent us from building nicer abstractions.

That being said, I might be mistaken about it, and maybe it's better to do occasional on-the-spot wrappings like here: https://github.com/crystal-lang/crystal/blob/master/src/process/executable_path.cr#L74

It's up to you and the community. What would be the process to reach a verdict here ?

Signed-off-by: Julien 'Lta' BALLET <contact@lta.io>

ysbaddaden · 2017-05-02T12:54:27Z

Pointers are required to bind to C libraries, they speed up implementations, both internally (for core/stdlib) and externally (for shards, apps); and without them we wouldn't achieve much, if anything. On the other hand, sysctl and ioctl are platform specific (it's not POSIX) and not required to build anything on, but specific use cases. We want a wide stdlib, covering general use cases, just not that wide and specific :)

elthariel · 2017-05-03T18:53:40Z

Hum... This is embarrassing. Just confirmed with the Single Unix Specification v4, sysctl isn't mentionned anywhere in the standard. I always thought it was part of SUS. As a sidenote,ioctl is defined by SUS/POSIC (but it's not the topic)

@ysbaddaden, To get back on the initial topic, I think sysctl is still the easier way to get the number of cores/cpus on OSX. So you'd advocate in favor of a hidden binding when it's needed and provide an higher level API for showing cpus/cores of a system ?

ysbaddaden · 2017-05-04T08:46:25Z

Do you have more examples and use cases that would benefit everyone? I mean other than the number of available CPU?

elthariel · 2017-05-04T15:41:34Z

I haven't really thought about it. That's an API I use from times on times with C and since I needed it for the number of cpu on OSX, I thought it would be a useful contribution. I'll have a look and think a bit about it if it can gives input for the community to decide whether it is or not

elthariel · 2017-05-04T15:53:14Z

I should precise that I'm more familiar with the Linux system APIs, which aren't based out of sysctl anymore, outside of a few idiomatic things you'll find here and here.

On the OSX side of things, it seems like a good way to get access to a bunch of system data about shared memory, VM, networking and hardware. I'm no OSX experts, so there might be better way to get access to this. We might also need input from OpenBSD and FreeBSD guys, but I think that might be the only way to access a whole bunch of similar system data. I think FreeBSD started to move to something similar to procfs, but I don't think OpenBSD was on that path (although I must confess the last time I worked with awesome openbsd was quite a while ago).

I certainly don't want to push on this topic, as you made pretty good points as of how this wouldn't be the most widely useful API. On the other hand, we'll need at least some portion of it internally so why not expose it ? TBH, I don't have anywhere near enough vision on Crystal's community and it's goals to have a strong opinion. You'll have to place the threshold :)

mverzilli · 2017-05-05T16:12:20Z

As @ysbaddaden said, this seems a very good fit for a separate shard:

It wraps a system specific tool.
Correct me if I'm wrong, but it doesn't seem to need "first class" compiler support or modifications.
We aren't sure if we want this in the stdlib yet, and would probably like to approach it based on use cases in a more platform agnostic way.

So, I'd put it in a shard, and if the ecosystem embraces it and it becomes a very common item in shard.yml files there's always time for the stdlib to "absorb" it.

elthariel · 2017-05-06T01:24:49Z

@mverzilli So be it. I'm not sure as I'm going to effectively create the shard as having it out of stdlib defeats the initial purpose of it.
Regarding this initial use case, I'm not sure there's a platform agnostic way to do it (i think it's quite the contrary), so we'd still have to implement something like this hidden somewhere

Add Sysctl interface support for OSX/Linux/FreeBSD

dbbfa9e

Signed-off-by: Julien 'Lta' BALLET <contact@lta.io>

Sija reviewed Apr 29, 2017

View reviewed changes

Sija suggested changes Apr 29, 2017

View reviewed changes

RX14 requested changes Apr 29, 2017

View reviewed changes

Sija reviewed Apr 29, 2017

View reviewed changes

Reviewers feedback (will be squashed)

0449221

Signed-off-by: Julien 'Lta' BALLET <contact@lta.io>

ysbaddaden reviewed May 1, 2017

View reviewed changes

@ysbaddaden feedback

a0425ad

Signed-off-by: Julien 'Lta' BALLET <contact@lta.io>

mverzilli closed this May 5, 2017

Uh oh!

Conversation

elthariel commented Apr 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elthariel Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RX14 Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oprypin Apr 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elthariel Apr 29, 2017 •

edited

Loading

RX14 Apr 29, 2017 •

edited

Loading

oprypin Apr 29, 2017 •

edited

Loading

elthariel commented May 3, 2017 •

edited

Loading