search

docker / go-units

Parse and print size and time units in human-readable format
https://godoc.org/github.com/docker/go-units
Apache License 2.0
218 stars 38 forks source link

Should verify that the max value is not too big #35

Open rhatdan opened 4 years ago

rhatdan commented 4 years ago

When parsing the ulimit, we should check if the ulimit max value is greater then the processes max value. If yes then we should return an error.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

rhatdan commented 4 years ago

@vrothberg FYI

vrothberg commented 4 years ago

@AkihiroSuda @vdemeester @thaJeztah PTAL

thaJeztah commented 4 years ago

There's been some discussion around validation in the "go" code versus delegating to the kernel itself; does Linux return an error in this case, or is it silently ignored?

rhatdan commented 4 years ago

Sure, the reason I wanted to do this hear was to avoid duplicating the data. Perhaps if we add a different function.

rhatdan commented 4 years ago

@thaJeztah Revamped this work, (Going through some old PR's) Now just added a verify function that will check against kernel. The problem is these values are just handed to different OCIs, and they can give you some useless information back to the user.

This should make it easier to diagnose the error, with a better error message.

rhatdan commented 4 years ago

@thaJeztah Decided to not wait 4 months again to update...

thaJeztah commented 2 years ago

@thaJeztah seems this issue was addressed -- the test case now gets the max value from the kernel.

Ah, forgot about this one.

I'm still a bit on the fence if querying what the kernel supports, and validating against that is within scope of this module and/or if it should be more of a concern of the code using this module (so far the module was used to only parse values, and to validate a correct format, but no other assumptions) 🤔.

What are your thoughts on that, @kolyshkin ?

rhatdan commented 2 years ago

Two year old PR, that I totally forgot about. :^(

kolyshkin commented 2 years ago

I'm on the verge about it. True, this package is only about parsing, it does not talk to the kernel in any way etc.

OTOH this package knows about ulimit, which is a UNIX concept, and adding Verify method is super easy here, and is not that easy in the other package (since the implementation uses a map which is internal to this package).

Overall I think this should go in, with the only change that Verify should not be specific to Linux -- looks like any unix (including *bsd and darwin) should support it. Perhaps even !windows (need to take a look at whatever x/sys/unix implements).

kolyshkin commented 2 years ago

Another thing is, this raises the bar to how this should be tested -- at least we now need to compile this for various GOOS/GOARCH combinations.

thaJeztah commented 2 years ago

Yes it's a bit of a tough one; I can definitely see the convenience of having the Validate(). Mostly concerned if it would be widening the scope too much (in addition to the dependency graph).