Skip to content

Add tests and fix for mdns NameSliceIter next_back - #104

Merged
ivmarkov merged 1 commit into
sysgrok:masterfrom
mlandauer:fix_mdns_nameslice_iter_labels
Sep 17, 2026
Merged

ivmarkov merged 1 commit into
sysgrok:masterfrom
mlandauer:fix_mdns_nameslice_iter_labels

Conversation

@mlandauer

Copy link
Copy Markdown
Contributor

The backwards labels iterator for NameSlice would always return None. This adds some tests for the forward and backwards iterator that would fail with the current implementation.

To fix the issue I added a second index for tracking the backwards iterator seperate from the forward iterator.

The docs for DoubleEndedIterator says
"It is important to note that both back and forth work on the same range, and do not cross: iteration is over when they meet in the middle."

Then a small bit of refactoring to extract common code from the back and next_back.

The backwards labels iterator for NameSlice would always return None.
This adds some tests for the forward and backwards iterator that would
fail with the current implementation.

To fix the issue I added a second index for tracking the backwards
iterator seperate from the forward iterator.

The docs for DoubleEndedIterator says
"It is important to note that both back and forth work on the same
range, and do not cross: iteration is over when they meet in the
middle."

Then a small bit of refactoring to extract common code from the back and
next_back.
@ivmarkov

Copy link
Copy Markdown
Collaborator

@mlandauer I'm so sorry - I was on a vacation at the time you opened the PR and I forgot about it upon my return. Let me review...

@mlandauer

Copy link
Copy Markdown
Contributor Author

@ivmarkov Thanks for letting me know. All good. Let me know if there's anything missing or needs adding. I also realise I haven't at all explained why I hit the problem I'm trying to fix. Don't know if that's helpful or not.

Copilot AI 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.

🟢 Approval recommended

The change is a small, self-contained, correct iterator bug fix that I verified across all cases (including empty slices and underflow safety) and that is backed by new forward, backward, and interleaved tests.

Pull request overview

This PR fixes a bug in edge-mdns's NameSliceIter, the DoubleEndedIterator over the labels of a NameSlice. Previously next_back() on a fresh iterator always returned None because it shared the single forward index (which starts at 0), making backward iteration unusable. The fix introduces a separate index_back cursor so forward and backward iteration operate over the same range without crossing, per the DoubleEndedIterator contract, and extracts the shared label-resolution logic into a label() helper. New unit tests cover forward, backward, and interleaved iteration.

I traced the new logic across forward-only, backward-only, interleaved, and empty-slice cases, and confirmed: the half-open range [index, index_back) with index_back initialized to len + 1 correctly yields all len labels plus the root label; index_back - 1 cannot underflow because the subtraction only runs when index_back != index; and the removed core::cmp::Ordering import has no remaining uses. The mod test naming and in-file test placement match the convention used in edge-http and edge-dhcp.

Changes:

  • Add a separate index_back cursor and initialize it to len + 1 so backward iteration works and meets forward iteration correctly in the middle.
  • Extract common label-resolution code into a NameSliceIter::label() helper used by both next() and next_back().
  • Add unit tests for forward, backward, and both-direction label iteration; remove the now-unused Ordering import.
File summaries
File Description
edge-mdns/src/lib.rs Adds index_back cursor and label() helper to fix next_back, initializes index_back in iter_labels, removes unused Ordering import, and adds tests for forward/backward/interleaved iteration.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@ivmarkov

Copy link
Copy Markdown
Collaborator

Thanks!

@ivmarkov
ivmarkov merged commit bd528e0 into sysgrok:master Sep 17, 2026
3 checks passed
@ivmarkov

Copy link
Copy Markdown
Collaborator

I also realise I haven't at all explained why I hit the problem I'm trying to fix. Don't know if that's helpful or not.

Certainly helpful if you explain it here, so that we have it as part of the PR tracking.

@mlandauer

Copy link
Copy Markdown
Contributor Author

I hit against the issue when I was trying to find mdns results matching a wildcard domain. Testing each found domain name (as a ParsedName) against a known domain using something like

name.ends_with(&NameSlice::new(&["_foo", "_udp", "local"]))

This would always give true whatever value name has, all because ends_with is implemented using the broken labels iterator.

ivmarkov added a commit to project-chip/rs-matter that referenced this pull request Sep 17, 2026
)

Subject says it all. Really two unrelated fixes, but since the memory
fix in the `respond` module was a one-liner, I combined it with the BTP
timeout fix.

EDIT 1: Also back-ported an mDNS fix from this:
sysgrok/edge-net#104
EDIT 2: The memory fix in `respond` (a port of
sysgrok/edge-net#105) _might_ have far-reaching
consequences, like completely getting rid of the [bump allocator in
downstream
crates](https://github.com/sysgrok/rs-matter-stack/blob/25113f98577c32e2b22feff3b6eb32a181d16e57/examples/light.rs#L50).
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