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

Y2038: Fix cache_peer connect-timeout reporting #1494

Closed
wants to merge 7 commits into from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Sep 25, 2023

... on systems with 64-bit time_t and 32-bit int.

Also fixed htcp=oldsquid reporting broken since 2015 commit 4ac1880.

This fix affects mgr:config and mgr:server_list cache manager reports.

Ensure that the timeout value in the neighbours
cachemgr report is not truncated on systems
having 32-bit integers
src/neighbors.cc Outdated Show resolved Hide resolved
@rousskov
Copy link
Contributor

I adjusted PR description to better match the proposed fix. Please adjust further as needed.

@rousskov rousskov added the S-waiting-for-author author action is expected (and usually required) label Sep 25, 2023
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 27, 2023
@rousskov rousskov self-requested a review October 2, 2023 13:31
@rousskov rousskov changed the title Y2038: correctly display neighbour in cachemgr Y2038: Fix cache_peer connect-timeout reporting Oct 2, 2023
rousskov
rousskov previously approved these changes Oct 2, 2023
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

I fixed one PR bug (commit eda290d) and polished, including PR title/description. Please review and adjust further as needed.

src/neighbors.cc Show resolved Hide resolved
src/neighbors.cc Outdated Show resolved Hide resolved
@rousskov
Copy link
Contributor

rousskov commented Oct 2, 2023

            p->connect_timeout_raw = xatoi(token + 16);

AFAICT, cache_peer connect-timeout parsing is similarly broken. Fixing that can be considered outside this PR scope, but I am not against adding that fix to this PR. Your call.

Co-authored-by: Alex Rousskov <[email protected]>
@kinkie
Copy link
Contributor Author

kinkie commented Oct 2, 2023

            p->connect_timeout_raw = xatoi(token + 16);

AFAICT, cache_peer connect-timeout parsing is similarly broken. Fixing that can be considered outside this PR scope, but I am not against adding that fix to this PR. Your call.

Let's keep this PR as a single focus. Added fixing the parsing to my TODO list

@kinkie kinkie added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Oct 2, 2023
src/neighbors.cc Show resolved Hide resolved
@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Oct 2, 2023
@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 2, 2023
@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels backport-to-v6 maintainer has approved these changes for v6 backporting and removed S-waiting-for-author author action is expected (and usually required) labels Oct 3, 2023
@kinkie kinkie self-assigned this Oct 3, 2023
squid-anubis pushed a commit that referenced this pull request Oct 3, 2023
... on systems with 64-bit time_t and 32-bit int.

Also fixed `htcp=oldsquid` reporting broken since 2015 commit 4ac1880.

This fix affects mgr:config and mgr:server_list cache manager reports.
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Oct 3, 2023
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 3, 2023
@kinkie kinkie removed the backport-to-v6 maintainer has approved these changes for v6 backporting label Nov 1, 2023
@kinkie
Copy link
Contributor Author

kinkie commented Nov 1, 2023

v6 backporting doesn't apply cleanly. Aborting backport

kinkie added a commit to kinkie/squid that referenced this pull request Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants