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

Jump to bottom

Proposal: flag: add support for int32 and uint32 options #26545

Closed
rvegas opened this issue Jul 23, 2018 · 6 comments
Closed

Proposal: flag: add support for int32 and uint32 options #26545

rvegas opened this issue Jul 23, 2018 · 6 comments
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone
Proposal

Comments

@rvegas
Copy link

rvegas commented Jul 23, 2018

Currently many core libraries like grpc have configurable flags and options that are either int32 or uint32, e.g.

This forces the userland code for any configurable system to look something like this:

...
maxConcurrentStreams := f.Int("max_streams", 100, "Number of maximum concurrent streams")
f.Parse()
...
maxConcurrentConnectionsOption := grpc.MaxConcurrentStreams(uint32(maxConcurrentStreams))
...

Such casting (and any other workaround) results in error prone implementations sometimes, since the error handling has to be done by the developer, which I'm not particulary against, but it's better if we could trust Go's core flag parsing library to support and validate this.

In this manner, we could allow the implementations to simply look like this:

maxConcurrentStreams := f.Uint32("max_streams", 100, "Number of maximum concurrent streams")
f.Parse()
...
maxConcurrentConnectionsOption := grpc.MaxConcurrentStreams(maxConcurrentStreams)
...

I know it may look simple but flag already supports int, int64 and float64, so imho, adding this makes sense and allows for better looking and more secure code.
@rvegas
Copy link
Author

rvegas commented Jul 23, 2018

I've opened #26465 with the necessary implementation to add this to flag.

@mvdan mvdan changed the title flag: add support for int32 and uint32 options Proposal: flag: add support for int32 and uint32 options Jul 23, 2018
@gopherbot gopherbot added this to the Proposal milestone Jul 23, 2018
@mvdan
Copy link
Member

mvdan commented Jul 23, 2018

I'm not sure that the presence of int64 and uint64 sets a precedent. I imagine they were added so that one can have numerical flags that accept very large values without having to worry about 32-bit builds.

@rvegas
Copy link
Author

rvegas commented Jul 23, 2018 •
edited

@mvdan the reason for adding uint32 and int32 is not due to the precedent of other types.

The main reason behinf all of this is to allow core libraries users to be able to produce homogeneous code that is less error prone in basic implementations.

I do agree with the thought of having added *64 types for build-related reasons tho.

@bcmills
Copy link
Contributor

bcmills commented Jul 23, 2018

You can already use flag.Var to declare a flag variable with an arbitrary validation function (the Set method), and flags in general may require different validation constraints: for example, some must be nonzero, or must fit in a uint32 after adding 12 (to account for some prefix added by the package), etc.

So what's special about the uint32 and int32 constraints? (If you believe they're more common than other validators, some statistics or examples would help.)

@bcmills
Copy link
Contributor

bcmills commented Jul 23, 2018

Re the boilerplate conversions being less secure, it seems like #19624 would address the same security concerns much more generally. If you have specific examples of security bugs caused by conversions missing range checks, please mention them there!

@bcmills bcmills added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 23, 2018
@rsc
Copy link
Contributor

rsc commented Jul 23, 2018

int/uint and int64/uint64 should be enough. We don't need the entire set. Note that if uint32 is a very important type (such as in this particular package) it's easy to build a uint32-accepting flag.Var, as @bcmills pointed out above.

@rsc rsc closed this as completed Jul 23, 2018
@ALTree ALTree mentioned this issue Jul 24, 2018
Closed
@golang golang locked and limited conversation to collaborators Jul 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
5 participants
@rsc @mvdan @bcmills @rvegas @gopherbot