Skip to content

Upgrade AIO::path String to SBuf - #2463

Open
yadij wants to merge 2 commits into
squid-cache:masterfrom
yadij:rm-AIOString-1
Open

Upgrade AIO::path String to SBuf#2463
yadij wants to merge 2 commits into
squid-cache:masterfrom
yadij:rm-AIOString-1

Conversation

@yadij

@yadij yadij commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Also, C++11 initialize the class to simplify constructor.

@yadij yadij changed the title Update AIO::path String to SBuf Upgrade AIO::path String to SBuf Jul 27, 2026

@kinkie kinkie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One comment; apart from that LGTM

Comment thread src/DiskIO/AIO/AIODiskFile.cc Outdated

@rousskov rousskov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All change requests are about out-of-scope PR changes.

Please do not post new non-emergency PRs until the backlog problem is solved.

Comment thread src/DiskIO/AIO/AIODiskFile.cc Outdated
Comment thread src/DiskIO/AIO/AIODiskFile.cc Outdated
AIODiskIOStrategy *strategy;
int fd = -1;
SBuf path;
AIODiskIOStrategy *strategy = nullptr;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this data member must be set by the class constructor, then it is best to leave it uninitialized here to avoid implying that some reasonable initial or default value exists:

Suggested change
AIODiskIOStrategy *strategy = nullptr;
AIODiskIOStrategy *strategy;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That practice being requested is a "bad practice".

Reason: The "undefined" value is completely random. Which means it may be a value within the application legitimately allocated memory and thus indistinguishable from a valid allocation when inspected by memory access checkers.

[ Given Squid memory pooling design, the allocation will be from a previously allocated object of the same type, meaning a high chance that the pointed to memory would have the structure of a legitimate (but not valid) strategy object. ]

Solution: Explicitly initialize to a value which is guaranteed to be outside accessible memory ranges. The value nullptr, NULL, or 0 are traditionally used for this.

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
@yadij
yadij requested a review from rousskov August 12, 2026 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants