Maintenance: update mcast_groups to SBufList #2312

yadij · 2025-11-29T22:28:55Z

No description provided.

kinkie

nice!

rousskov

I plan to come back to this new PR after the backlog is cleared. FWIW, my earlier repeated requests to stop posting such PRs still stand.

P.S. I removed "Maintenance" prefix from PR title because this PR does change running code in non-trivial ways.

rousskov · 2025-11-30T15:11:44Z

src/SquidConfig.h

    size_t tcpRcvBufsz;
    size_t udpMaxHitObjsz;
-    wordlist *mcast_group_list;
+    SBufList mcast_group_list;


Please do not add non-POD data members to SquidConfig. That class must be a POD because the global configuration object is accessed before main() starts and after main() exits¹. Our NoNewGlobals policy essentially applies to SquidConfig data members. SquidConfig is already documented to be a POD. Several existing data members violate this rule and that documentation², but we should not make the problem more difficult to fix by adding new violations.

AFAICT, the correct new type for mcast_group_list data member is a raw pointer to SBufList. Unfortunately, more code changes are required to support that new type in current code, but nothing major.

Footnotes

For an example of problems caused by non-POD Config members, see recent commit c565067 that fixes one of those problems. ↩

We probably just did not know better when we added them. ↩

FYI: SquidConfig has contained other SBufList for a year now without problems. So has already been proven safe for this particular global usage.

FTR: the issues requiring strictly POD-only members have been resolved since Squid v4 (3b1439d). Objects which use MEMPROXY allocation and are default constructable can be used, at least as members of existing globals now.

The issue(s) behind c565067 were that values within the post initialization SSL_CTX are not global safe. The ContextPointer itself should be global-safe, but the context it was pointing to needed to be deleted explicitly at a certain timing/order.

FTR: the issues requiring strictly POD-only members have been resolved since Squid v4 (3b1439d).

The "issues" are C++ global object construction and destruction rules. They cannot be resolved without changing the language itself.

src/icp_v2.cc

yadij · 2025-11-30T17:45:14Z

P.S. I removed "Maintenance" prefix from PR title because this PR does change running code in non-trivial ways.

"trivial" is not required for something to be maintenance. FYI, this PR is enacting software maintenance process step 5 migration - deprecated/obsolete wordlist is being replaced.

That step is also commonly called/described as "preventative maintenance: Refactoring the codebase to improve efficiency, readability, and maintainability, and make it less prone to errors."

kinkie · 2025-11-30T18:53:20Z

I agree with keeping the PR title as is

rousskov

Alex: I removed "Maintenance" prefix from PR title because this PR does change running code in non-trivial ways.

Amos: "trivial" is not required for something to be maintenance.

Very true, but I am against using "Maintenance" PR title prefix for such non-trivial changes. IIRC, I have already detailed the reasons behind my preference, probably more than once.

Francesco: I agree with keeping the PR title as is

Sigh. Counting votes is not how we should resolve differences in opinion, IMHO.

rousskov · 2025-11-30T20:52:19Z

src/SquidConfig.h

    size_t tcpRcvBufsz;
    size_t udpMaxHitObjsz;
-    wordlist *mcast_group_list;
+    SBufList mcast_group_list;


FTR: the issues requiring strictly POD-only members have been resolved since Squid v4 (3b1439d).

The "issues" are C++ global object construction and destruction rules. They cannot be resolved without changing the language itself.

kinkie · 2025-12-01T19:56:59Z

Francesco: I agree with keeping the PR title as is

Sigh. Counting votes is not how we should resolve differences in opinion, IMHO.

It can be if other options fail, but that was not the message I was aiming for.
Let me try to word it differently:

I don't think such a trivial change is something that is worth holding up a PR on.
As such, I will defer to whatever the author of the PR picks, as they have put in the work, their code beats my opinions anyway.

rousskov · 2025-12-01T20:15:51Z

I don't think such a trivial change is something that is worth holding up a PR on. As such, I will defer to whatever the author of the PR picks, as they have put in the work, their code beats my opinions anyway.

Understood. Thank you for confirming that my attempts to explain why (or even relay that) title prefixes are not a trivial matter for me have failed.

kinkie · 2025-12-01T20:50:06Z

I don't think such a trivial change is something that is worth holding up a PR on. As such, I will defer to whatever the author of the PR picks, as they have put in the work, their code beats my opinions anyway.

Understood. Thank you for confirming that my attempts to explain why (or even relay that) title prefixes are not a trivial matter for me have failed.

Trivial != unimportant. I don't think they're trivial but if I am to list components of a PR in descending order of importance for me, I have:

the code (duh)
code tests
PR title (without prefixes)
PR description
code comments
PR discussion
PR prefixes

Nothing is trivial, but for me, I'd generally request changes for 1-3, suggest changes for 4-5 and maybe drop a comment for 7 (6 is important but can't be changed, obviously)

rousskov · 2025-12-01T22:20:01Z

I don't think such a trivial change is something that is worth holding up a PR on. As such, I will defer to whatever the author of the PR picks, as they have put in the work, their code beats my opinions anyway.

Understood. Thank you for confirming that my attempts to explain why (or even relay that) title prefixes are not a trivial matter for me have failed.

Trivial != unimportant. I don't think they're trivial but if I am to list components of a PR in descending order of importance for me, I have:
1. ...
7. PR prefixes 
Nothing is trivial, but for me, I'd generally request changes for 1-3, suggest changes for 4-5 and maybe drop a comment for 7 (6 is important but can't be changed, obviously)

I understand that prefixes are the least important item for you. I wish the fact that prefixes are important for me would change your authors-choice treatment, but I obviously lost that battle.

kinkie · 2025-12-01T22:37:26Z

I understand that prefixes are the least important item for you. I wish the fact that prefixes are important for me would change your authors-choice treatment, but I obviously lost that battle.

You may have failed to convince me that it's worthwhile holding up a PR on prefixes, but at the same time I'm not forcing your (or anyone else's) behaviour. It's fine to have different viewpoints

rousskov · 2025-12-02T14:26:46Z

I understand that prefixes are the least important item for you. I wish the fact that prefixes are important for me would change your authors-choice treatment, but I obviously lost that battle.

You may have failed to convince me that it's worthwhile holding up a PR on prefixes, but at the same time I'm not forcing your (or anyone else's) behaviour.

I was hoping for your assistance with reducing my overheads¹, but "not forcing your behaviour" is certainly better than some of the alternatives I can easily imagine. Thank you for that!

convince me that it's worthwhile holding up a PR on prefixes

Just to avoid a misunderstanding: My negative votes for this PR were not due to this PR title prefix.

In an area where others did not voice strong preferences. ↩

Maintenance: update mcast_groups to SBufList

1ea9e13

yadij added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Nov 29, 2025

kinkie previously approved these changes Nov 29, 2025

View reviewed changes

rousskov self-requested a review November 30, 2025 14:43

rousskov changed the title ~~Maintenance: update mcast_groups to SBufList~~ update mcast_groups to SBufList Nov 30, 2025

rousskov requested changes Nov 30, 2025

View reviewed changes

rousskov removed M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Nov 30, 2025

Reviewer requested change

2cb7271

yadij dismissed kinkie’s stale review via 2cb7271 November 30, 2025 17:23

yadij changed the title ~~update mcast_groups to SBufList~~ Maintenance: update mcast_groups to SBufList Nov 30, 2025

yadij requested a review from rousskov November 30, 2025 17:45

kinkie approved these changes Nov 30, 2025

View reviewed changes

rousskov requested changes Nov 30, 2025

View reviewed changes

Maintenance: update mcast_groups to SBufList #2312

Are you sure you want to change the base?

Maintenance: update mcast_groups to SBufList #2312

Uh oh!

Conversation

yadij commented Nov 29, 2025

Uh oh!

kinkie left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov Nov 30, 2025

Choose a reason for hiding this comment

Footnotes

Uh oh!

yadij Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

yadij Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

rousskov Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yadij commented Nov 30, 2025

Uh oh!

kinkie commented Nov 30, 2025

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

kinkie commented Dec 1, 2025

Uh oh!

rousskov commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinkie commented Dec 1, 2025

Uh oh!

rousskov commented Dec 1, 2025

Uh oh!

kinkie commented Dec 1, 2025

Uh oh!

rousskov commented Dec 2, 2025

Footnotes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rousskov left a comment •

edited

Loading

rousskov commented Dec 1, 2025 •

edited

Loading