-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Avoid silently overflowing (signed) ints to unsigned int values in args #3317
Conversation
@@ -98,12 +125,12 @@ static int parse_opts(int argc, char** argv, options_t* opt) | |||
{ | |||
switch(id) | |||
{ | |||
case OPT_THREADS: opt->threads = atoi(s.arg_val); break; | |||
case OPT_MINTHREADS: opt->min_threads = atoi(s.arg_val); minthreads_set = true; break; | |||
case OPT_THREADS: if(parse_uint(&opt->threads, 1, s.arg_val)) err_out(id, "can't be less than 1"); break; |
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.
Should this be 0
or 1
as the min value? 0
(or unspecified) used to mean physical core count… but did people really use 0
explicitly to mean that?
I like to hope not, and I think this is a more explicit behavior.
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.
So we can scale down to 0 scheduler threads which means that minthreads is ok to be set to 0.
ponythreads with scaling on is really a maxthreads.
So, I think 1 as probably because, at 0, nothing can ever happen. we need to be able to do at least 1.
This does raise an interesting question. Should |
@alexsnaps I believe this is correct and ready to be merged, yes? |
Two questions:
|
Don't worry about formatting for now. CHANGELOG.... yeah I think an entry would be good. |
Added — I think this is good for merge. |
Thanks, @alexsnaps. Can you add release notes to the issue for the next release? |
Follow up to #3303
Also, I think I should add a commit to handle the formatting around
{
on the same line as their matching…if
statements? All statements? What is the formatting preference? Should I bring consistency to the entire file? See #3303 (comment)Finally, I think c429d54 probably warrants a CHANGELOG entry, wdyt?