Skip to content

Fix index issue with records in HostDBRecord::select_best_srv#13132

Open
cmcfarlen wants to merge 2 commits intoapache:masterfrom
cmcfarlen:fix-loop-best-srv
Open

Fix index issue with records in HostDBRecord::select_best_srv#13132
cmcfarlen wants to merge 2 commits intoapache:masterfrom
cmcfarlen:fix-loop-best-srv

Conversation

@cmcfarlen
Copy link
Copy Markdown
Contributor

@cmcfarlen cmcfarlen commented Apr 30, 2026

This came up in a copilot review of a fix unrelated to this issue so I created this separate PR to address this.

It seems the loop structure at some point looped over an index, but was changed to a range based loop, neglecting to change one aspect. But it was not noticed because the index variable is declared before the loop.

The issue is the is_down is always checked on the first target, so could skip up targets or use down targets depending.

@cmcfarlen cmcfarlen requested a review from Copilot April 30, 2026 18:37
@cmcfarlen cmcfarlen self-assigned this Apr 30, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a subtle selection bug in HostDBRecord::select_best_srv where a stale index variable from a prior index-based loop was still being used after converting the loop to range-based iteration, potentially causing incorrect skipping of SRV targets.

Changes:

  • Remove the unused / stale loop index variable and stop indexing rr[i] inside the range-based loop.
  • Use the range-loop reference (target) consistently for is_down() checks.
  • Re-scope the i index to the weighted-selection loop where it is actually needed.

Comment thread src/iocore/hostdb/HostDB.cc Outdated
Comment thread src/iocore/hostdb/HostDB.cc Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

@masaori335 masaori335 left a comment

Choose a reason for hiding this comment

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

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants