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

Use str* API to check for valid numeric value #677

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aquette
Copy link
Member

@aquette aquette commented Mar 13, 2019

Closes #676

Signed-off-by: Arnaud Quette ArnaudQuette@eaton.com

Closes networkupstools#676

Signed-off-by: Arnaud Quette <ArnaudQuette@eaton.com>
@aquette aquette requested review from clepple and zykh March 13, 2019 15:34
Copy link
Contributor

@zykh zykh left a 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.

server/conf.c Outdated Show resolved Hide resolved
Signed-off-by: Arnaud Quette <ArnaudQuette@eaton.com>
Signed-off-by: Arnaud Quette <ArnaudQuette@eaton.com>
@aquette
Copy link
Member Author

aquette commented Mar 15, 2019

@zykh fixed the both

Copy link
Member

@clepple clepple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@zykh zykh left a 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:

    nut/server/upsd.c

    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 now certrequest is unsigned and NETSSL_CERTREQ_NO is zero, just if (certrequest > NETSSL_CERTREQ_REQUIRE)) ?
    (i.e. || instead of &&, and NETSSL_CERTREQ_REQUIRE, which is 2, instead of NETSSL_CERTREQ_REQUEST, which is 1)

    nut/server/netssl.c

    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 (as long) exceeds what can be stored in an unsigned int?
    here:
    ret = sysconf(_SC_OPEN_MAX);

    and here:

    nut/server/upsd.c

    Line 1251 in 3d79006

    maxconn = sysconf(_SC_OPEN_MAX);

@clepple clepple self-requested a review April 5, 2019 11:08
Copy link
Member

@clepple clepple left a 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.

@jimklimov
Copy link
Member

jimklimov commented Oct 9, 2020

@aquette @clepple : a few merge conflicts cropped up over time :( Probably your versions should be trusted, but can you please revise that this is the case - and that master-branch changes so far did not change non-trivially some semantics etc.?

@jimklimov jimklimov added this to the 2.8.1 milestone Apr 26, 2022
@jimklimov jimklimov added the refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings label Aug 19, 2022
@jimklimov jimklimov modified the milestones: 2.8.1, 2.8.2 Jan 6, 2023
@jimklimov jimklimov modified the milestones: 2.8.2, 2.8.3 Apr 4, 2024
@jimklimov jimklimov added the C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks label Apr 10, 2024
@jimklimov jimklimov modified the milestones: 2.8.3, 2.8.4 Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-str Issues and PRs about C/C++ methods, headers and data types dealing with strings and memory blocks merge-conflicts refactor/fightwarn PR or issue proposal to improve code maintainability without functional changes, or to fix warnings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use str* API in place of isdigit() in server/conf.c
4 participants