Skip to content

Add DSCP class-name keyword support#492

Open
yaakov-stein wants to merge 3 commits intofacebook:mainfrom
yaakov-stein:add_dscp_keywords
Open

Add DSCP class-name keyword support#492
yaakov-stein wants to merge 3 commits intofacebook:mainfrom
yaakov-stein:add_dscp_keywords

Conversation

@yaakov-stein
Copy link
Copy Markdown
Contributor

As mentioned in #383, add support for DSCP class name keywords in ip4.dscp and ip6.dscp matchers. Instead of requiring users to memorize numeric DSCP codepoints, rules can now use standard class names:

ip4.dscp eq ef
ip6.dscp not cs1
ip4.dscp eq af21

Supported keywords follow the IANA DSCP registry and match iptables conventions (see dscp_helper.c):

  • CS0–CS7 (Class Selector)
  • AF11–AF43 (Assured Forwarding)
  • EF (Expedited Forwarding)
  • BE (Best Effort, alias for CS0)

Keywords are case-insensitive. Numeric values (0–63, decimal or hex) continue to work as before.

Testing

  • Unit + e2e tests
  • Manual Testing

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 30, 2026

Claude review of PR #492 (e0408af)

Suggestions

  • Add not operator e2e test for class names (ip4)tests/e2e/parsing/ip4_dscp.sh:30 — Class name keywords are only tested with eq; adding a not ef test would cover both operators at the integration level.
  • Add not operator e2e test for class names (ip6)tests/e2e/parsing/ip6_dscp.sh:31 — Same gap as ip4: no not operator test with a class name keyword.
  • Move public conversion functions to match file organizationsrc/libbpfilter/matcher.c:732bf_dscp_class_to_str and bf_dscp_class_from_str are placed between static helpers, while all other public conversion functions are grouped near the bottom of the file.
  • Fold "be" alias into lookup tablesrc/libbpfilter/matcher.c:1706 — Adding {"be", 0} to _bf_dscp_classes would let the generic loop handle it, removing the special-case branch. bf_dscp_class_to_str(0) would still return "cs0" since it appears first in the array.

Nits

  • Misleading "out-of-range" commenttests/unit/libbpfilter/matcher.c:394 — Value 64 is within uint8_t range; "values without a named class" would be more precise.
  • Add generic invalid string testtests/unit/libbpfilter/matcher.c:414dscp_class_conversions only tests class-name-like invalid inputs; adding a generic "invalid" string would match the ipproto_conversions pattern.
  • Unnecessary (uint8_t *) castsrc/libbpfilter/matcher.c:740 — Other parse functions pass payload directly to *_from_str without a cast; the explicit cast is safe but inconsistent with the pattern.
  • HTTP URL in commentsrc/libbpfilter/matcher.c:1677 — The IANA link uses plain HTTP; consider updating to the HTTPS URL.

CLAUDE.md improvements

  • Several public conversion function pairs in matcher.h (e.g. bf_ethertype_to_str, bf_ipproto_to_str, bf_icmp_type_to_str) lack Doxygen documentation. The new bf_dscp_class_* functions follow this existing pattern, but the style guide recommends documenting non-trivial public API functions. Consider adding a note to CLAUDE.md or the style guide clarifying documentation expectations for *_to_str/*_from_str conversion functions.

Workflow run

Allow DSCP matchers (ip4.dscp, ip6.dscp) to accept symbolic class
names (cs0-cs7, af11-af43, ef, be) in addition to numeric values.
The class name table is based on iptables' dscp_helper.c and the
IANA DSCP registry. Matching is case-insensitive.
Add unit tests for bf_dscp_class_to_str/bf_dscp_class_from_str
conversion functions, keyword parsing in ip4_dscp and ip6_dscp
matchers, and e2e tests exercising class name keywords with both
valid and invalid inputs.
assert(payload);
assert(raw_payload);

if (!bf_dscp_class_from_str(raw_payload, (uint8_t *)payload))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude: nit: Other _bf_parse_* functions pass payload directly to their corresponding *_from_str call without a cast (e.g. bf_icmp_type_from_str(raw_payload, payload), bf_ipproto_from_str(raw_payload, payload)). The explicit (uint8_t *) cast is safe but unnecessary in C (void * converts implicitly) and inconsistent with the existing pattern.

assert(str);
assert(dscp);

if (bf_streq_i(str, "be")) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude: suggestion: The "be" alias is handled as a special case before the main loop. Adding {"be", 0} at the end of _bf_dscp_classes would fold it into the generic loop, removing this branch. bf_dscp_class_to_str(0) would still return "cs0" because it appears first in the array, preserving the canonicalization behavior.

}

/* DSCP class name to codepoint mapping, based on iptables
* dscp_helper.c and http://www.iana.org/assignments/dscp-registry.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude: nit: The IANA link uses plain HTTP. Consider updating to https://www.iana.org/assignments/dscp-registry/dscp-registry.xhtml.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant