-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Use str* API to check for valid numeric value #677
base: master
Are you sure you want to change the base?
Conversation
Closes networkupstools#676 Signed-off-by: Arnaud Quette <ArnaudQuette@eaton.com>
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.
As previously mentioned, I still think that we should also make maxage
, and friends, unsigned int
.
Signed-off-by: Arnaud Quette <ArnaudQuette@eaton.com>
Signed-off-by: Arnaud Quette <ArnaudQuette@eaton.com>
@zykh fixed the both |
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.
LGTM
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.
Just a couple of things:
- printf formats:
Lines 699 to 702 in 3d79006
fatalx(EXIT_FAILURE, "Your system limits the maximum number of connections to %d\n" "but you requested %d. The server won't start until this\n" "problem is resolved.\n", ret, maxconn); - shouldn't the following be
if (certrequest < NETSSL_CERTREQ_NO || certrequest > NETSSL_CERTREQ_REQUIRE)
(or, given that nowcertrequest
is unsigned andNETSSL_CERTREQ_NO
is zero, justif (certrequest > NETSSL_CERTREQ_REQUIRE)
) ?
(i.e.||
instead of&&
, andNETSSL_CERTREQ_REQUIRE
, which is 2, instead ofNETSSL_CERTREQ_REQUEST
, which is 1)
Lines 481 to 485 in 3d79006
if (certrequest < NETSSL_CERTREQ_NO && certrequest > NETSSL_CERTREQ_REQUEST) { upslogx(LOG_ERR, "Invalid certificate requirement"); return; } - how unlikely is that
sysconf(_SC_OPEN_MAX)
returns a)-1
or b) a value that (aslong
) exceeds what can be stored in anunsigned int
?
here:
Line 696 in 3d79006
ret = sysconf(_SC_OPEN_MAX);
and here:
Line 1251 in 3d79006
maxconn = sysconf(_SC_OPEN_MAX);
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.
In light of @zykh's comments, I think the change to unsigned
is hasty. As pointed out, the sysconf()
call shouldn't return a negative number, but a check is not expensive.
Closes #676
Signed-off-by: Arnaud Quette ArnaudQuette@eaton.com