Skip to content

Comments

Cleanup helper.h classes#750

Open
yadij wants to merge 4 commits intosquid-cache:masterfrom
yadij:cvs-1461141
Open

Cleanup helper.h classes#750
yadij wants to merge 4 commits intosquid-cache:masterfrom
yadij:cvs-1461141

Conversation

@yadij
Copy link
Contributor

@yadij yadij commented Nov 15, 2020

Polish up the classes in helper.h to use proper constructors as the
caller API for creation + initialization. Use C++11 initialization for
default values.

  • no "virtual" in helper class destructor declaration could create
    memory leaks in future (poorly) refactored code; the gained protection
    is probably worth adding the (currently unused) virtual table to the
    helper class; resolves Coverity issue CID 1461141 (VIRTUAL_DTOR)
  • missing Comm::Connection timers initialization on helper startup
  • multiple initialization of values used for state accounting
  • initialization of booleans to non-0 integer values
  • initialization of char using numeric values
  • missing mgr:filedescriptors description for helper sockets

yadij and others added 4 commits November 15, 2020 14:48
Setting this member as a side-effect of rbuf initialization
when it is listed as a class member later than rbuf caused
the value to be overwritten with 0 in the construct sequence.
@yadij yadij added the S-waiting-for-QA QA team action is needed (and usually required) label Nov 15, 2020
@yadij
Copy link
Contributor Author

yadij commented Nov 15, 2020

This is a repeat of PR #719 with fixes by Christos, and additional debugging outputs by myself to review the helper I/O traffic.

I think we have the problems ironed out properly. But would still like some third-party testing to confirm no more weirdness happens before clearing for merge.

@rousskov rousskov added S-waiting-for-more-reviewers needs a reviewer and/or a second opinion and removed S-waiting-for-QA QA team action is needed (and usually required) labels Dec 9, 2020
@rousskov
Copy link
Contributor

rousskov commented Dec 9, 2020

Amos would still like some third-party testing to confirm no more weirdness happens before clearing for merge.

rousskov added S-waiting-for-more-reviewers and removed S-waiting-for-QA labels

S-waiting-for-QA is for PRs that need QA team attention -- that team needs to add or fix something before the PR can be merged. The current automated tests do not cover helpers, and I doubt Amos would want this PR to wait until they do.

We do not have a GitHub label that covers "3rd party testing" waiting well, so I used S-waiting-for-more-reviewers. We can add S-waiting-3rd-party and use it here instead.

@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 Jan 8, 2021
@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 Feb 2, 2021
@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 Feb 11, 2021
@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 Feb 19, 2021
@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 Feb 28, 2021
squid-anubis pushed a commit that referenced this pull request Mar 4, 2021
Polish up the classes in helper.h to use proper constructors as the
caller API for creation + initialization. Use C++11 initialization for
default values.

* no "virtual" in helper class destructor declaration could create
  memory leaks in future (poorly) refactored code; the gained protection
  is probably worth adding the (currently unused) virtual table to the
  helper class; resolves Coverity issue CID 1461141 (VIRTUAL_DTOR)
* missing Comm::Connection timers initialization on helper startup
* multiple initialization of values used for state accounting
* initialization of booleans to non-0 integer values
* initialization of char using numeric values
* missing mgr:filedescriptors description for helper sockets
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-failed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels labels Mar 4, 2021
@squid-anubis squid-anubis removed the M-passed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Mar 15, 2021
@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 15, 2021
@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 Feb 13, 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 Mar 27, 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 Jul 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 Jan 9, 2024
@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 Apr 5, 2024
@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 Jun 3, 2024
@kinkie kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

M-failed-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels S-waiting-for-more-reviewers needs a reviewer and/or a second opinion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants