Skip to content

Conversation

@breuhan
Copy link

@breuhan breuhan commented Jan 12, 2026

No description provided.

@breuhan breuhan requested a review from ip1981 January 12, 2026 18:52
@breuhan breuhan self-assigned this Jan 12, 2026
@breuhan breuhan added the enhancement New feature or request label Jan 12, 2026
@breuhan breuhan requested a review from Copilot January 12, 2026 18:55
Copy link

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

This PR adds support for parsing IPv6 addresses by implementing hex range parsing capabilities. The implementation follows the existing decimal range parsing patterns and integrates seamlessly with the current hostlist parser architecture.

Changes:

  • Added hex range parsing support to enable IPv6 address expansion (e.g., 2001:db8::[0-f])
  • Extended host element parser to accept colon characters for IPv6 address literals
  • Added comprehensive test coverage for IPv4 and IPv6 address parsing scenarios

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

File Description
src/structures.rs Added HexRange variants and format_hex_prefix function to support hexadecimal range formatting
src/lib.rs Implemented hex parsing functions (hex_digits, leading_hex, range_hex, disjoint_hex) and integrated them into the range parser; added IPv4/IPv6 integration tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 606 to 616
#[test]
fn test_parse_ip_addresses() {
assert_debug_snapshot!("IPv4 single", parse("192.168.0.1"));
assert_debug_snapshot!("IPv6 compressed", parse("2001:db8::1"));
assert_debug_snapshot!(
"IPv6 full",
parse("fe80:1234:5678:9abc:def0:1234:5678:9abc")
);
assert_debug_snapshot!("Multiple IPv6 literals", parse("2001:db8::1, 2001:db8::2"));
assert_debug_snapshot!("IPv6 expansion", parse("2001:db8::[0-f]"));
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The new hex parsing functions (leading_hex, range_hex, disjoint_hex) lack unit tests similar to their decimal equivalents (test_leading_zeros, test_range_digits, test_disjoint_digits). While integration tests are provided, unit tests would help ensure each parser component works correctly in isolation and make debugging easier if issues arise. Consider adding unit tests that directly test these functions.

Copilot uses AI. Check for mistakes.
Comment on lines 606 to 616
#[test]
fn test_parse_ip_addresses() {
assert_debug_snapshot!("IPv4 single", parse("192.168.0.1"));
assert_debug_snapshot!("IPv6 compressed", parse("2001:db8::1"));
assert_debug_snapshot!(
"IPv6 full",
parse("fe80:1234:5678:9abc:def0:1234:5678:9abc")
);
assert_debug_snapshot!("Multiple IPv6 literals", parse("2001:db8::1, 2001:db8::2"));
assert_debug_snapshot!("IPv6 expansion", parse("2001:db8::[0-f]"));
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The test suite doesn't include a test case for hex disjoint ranges (e.g., parse("2001:db8::[a,b,c]")). While the "IPv6 expansion" test covers hex ranges with the dash operator, adding a test for comma-separated hex values would provide more complete coverage of the disjoint_hex parser functionality.

Copilot uses AI. Check for mistakes.
Comment on lines 606 to 616
#[test]
fn test_parse_ip_addresses() {
assert_debug_snapshot!("IPv4 single", parse("192.168.0.1"));
assert_debug_snapshot!("IPv6 compressed", parse("2001:db8::1"));
assert_debug_snapshot!(
"IPv6 full",
parse("fe80:1234:5678:9abc:def0:1234:5678:9abc")
);
assert_debug_snapshot!("Multiple IPv6 literals", parse("2001:db8::1, 2001:db8::2"));
assert_debug_snapshot!("IPv6 expansion", parse("2001:db8::[0-f]"));
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The test suite doesn't include a test case for reversed hex ranges (e.g., parse("2001:db8::[f-0]")). While reversed decimal ranges are tested (line 466), adding a test for reversed hex ranges would provide more complete coverage of the HexRangeReversed functionality.

Copilot uses AI. Check for mistakes.
Comment on lines +86 to +102
pub(crate) fn format_hex_prefix(num: u64, prefix: usize, same_prefix_len: bool) -> String {
let s = format!("{:x}", num);
let width = if same_prefix_len {
prefix + s.len()
} else {
prefix + 1
};

if width <= s.len() {
s
} else {
let mut out = String::with_capacity(width);
out.extend(std::iter::repeat_n('0', width - s.len()));
out.push_str(&s);
out
}
}
Copy link

Copilot AI Jan 12, 2026

Choose a reason for hiding this comment

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

The test suite in structures.rs only tests the decimal range variants (Range, Disjoint) but doesn't test the new hex variants (HexRange, HexRangeReversed, HexDisjoint). Consider adding tests similar to test_range_output_range_iter and test_range_output_disjoint_iter for the hex variants to ensure the iterator implementation and formatting work correctly.

Copilot uses AI. Check for mistakes.
@breuhan breuhan requested a review from spoutn1k January 12, 2026 19:01
@palash3 palash3 self-requested a review January 13, 2026 06:33
Copy link

@palash3 palash3 left a comment

Choose a reason for hiding this comment

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

Bug:

In ipv6 hostlist 1-10 is treated as decimal instead of hexadecimal.
So only 10 ips are generated instead of 16.

@spoutn1k
Copy link

The changes allow us to specify bogus addresses

Reviewing [1/1] [email protected]:
Snapshot file: src/snapshots/hostlist_parser__tests__IPv4 with v6 range.snap
Snapshot: IPv4 with v6 range.snap
Source: src/lib.rs:622
───────────────────────────────────────────────────────────────────────────────────────────────
Expression: parse("192.168.0.[0-f]")
───────────────────────────────────────────────────────────────────────────────────────────────
+new results
────────────┬──────────────────────────────────────────────────────────────────────────────────          
          1 │+Ok(
          2 │+    [
          3 │+        "192.168.0.0",
          4 │+        "192.168.0.1",
          5 │+        "192.168.0.2",
          6 │+        "192.168.0.3",
          7 │+        "192.168.0.4",
          8 │+        "192.168.0.5",
          9 │+        "192.168.0.6",
         10 │+        "192.168.0.7",
         11 │+        "192.168.0.8",
         12 │+        "192.168.0.9",
         13 │+        "192.168.0.a",
         14 │+        "192.168.0.b",
         15 │+        "192.168.0.c",
         16 │+        "192.168.0.d",
         17 │+        "192.168.0.e",
         18 │+        "192.168.0.f",
         19 │+    ],
         20 │+)
────────────┴──────────────────────────────────────────────────────────────────────────────────

@breuhan
Copy link
Author

breuhan commented Jan 13, 2026

The changes allow us to specify bogus addresses

No thats not by the change, that is already the case. Remember the hostlist operates on FQDNs and supports IPv4 by accident.

@spoutn1k
Copy link

spoutn1k commented Jan 13, 2026

Yeah, but I think the drift between IPv4/fqdn and IPv6 is greater. a.b.c.d can be amazon.fr.or.us and 192.168.1.1 but : is very much specific to IPv6. I think we should treat IPv6 as its special case.

Also before the change you could only add digits in the range so the above example would have been valid no ?

);
assert_debug_snapshot!("Multiple IPv6 literals", parse("2001:db8::1, 2001:db8::2"));
assert_debug_snapshot!("IPv6 expansion", parse("2001:db8::[0-f]"));

Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
assert_debug_snapshot!("IPv6 expansion with base 16", parse("2001:db8::[00-10]"));

This should also work. But maybe it makes sense to limit that. Its very easy to generate billions of addresses.

Choose a reason for hiding this comment

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

For some reason the hex functions allowed only strings with alphabetic characters ? Removed this and your test works.

Copy link
Author

Choose a reason for hiding this comment

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

What are your thoughts on limiting the amount of expansion to 2-3 chars? I'm not sure.

Copy link

Choose a reason for hiding this comment

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

Inappropriate usage of these wild cards is always going to be an issue.

Copy link

@spoutn1k spoutn1k Jan 15, 2026

Choose a reason for hiding this comment

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

I mean we can easily count a range and abort if the amount is greater than a threshold

@spoutn1k spoutn1k marked this pull request as ready for review January 15, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants