-
Notifications
You must be signed in to change notification settings - Fork 904
DECnet (again) and test gaps #1564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
infrastation
merged 6 commits into
the-tcpdump-group:master
from
infrastation:fix_decnet_again
Oct 16, 2025
Merged
DECnet (again) and test gaps #1564
infrastation
merged 6 commits into
the-tcpdump-group:master
from
infrastation:fix_decnet_again
Oct 16, 2025
+256
−177
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Even after my earlier commit 989e194 the current implementation still fails to reject some invalid forms of DECnet syntax: 'decnet net net-10-20-30-0.libpcap.test': no error 'decnet net 1.2.3.0/24': no error 'decnet net 1.2.3.0 mask 255.255.255.0': no error 'decnet net 1.2': no error Make the verification stricter to solve the remaining part of the problem: in gen_scode() and gen_mcode() handle DECnet early and reject all forms, in gen_ncode() handle DECnet early and reject all forms except "host". Improve various comments to explain the context. For a number of other "decnet" forms instead of misleading error messages (e.g., "Mask syntax for networks only") return a more useful error message: 'decnet net net-10-20-30-0.libpcap.test': 'decnet' is not a valid qualifier for 'net' 'decnet net 1.2.3.0/24': 'decnet' is not a valid qualifier for 'net' 'decnet net 1.2.3.0 mask 255.255.255.0': 'decnet' is not a valid qualifier for 'net' 'decnet net 1.2': 'decnet' is not a valid qualifier for 'net'
Unlike gen_ncode(), gen_mcode() and gen_mcode6(), there is exactly one code path into gen_scode(), namely, through an ID in the grammar. This way, when a comment says "ID -> gen_scode()", it states the obvious, and when it says "pnum -> gen_scode()", it is wrong, so eliminate this noise to improve the signal. Add the missing gen_host46_byname() to a couple code paths.
The existing tests could not detect the recently fixed DECnet bugs because the definitions in %pqual_features do not distinguish between different forms of the ID. Then the subsequent foreach loop that generates invalid "host" primitives does not generate any tests for protocols that support a form of "host" no matter which forms of ID are valid and which are not. Then the resulting reject tests have a number of gaps, including IPv4 IDs in DECnet primitives. To fix that, in %pqual_features replace the generic "host" with seven more specific host/net features; in the loop skip the valid permutations only and include MAC-48, ARCnet and DECnet IDs. The new test space supersedes a number of earlier reject tests, so remove these. Document the current code paths in the comments and introduce more helper functions.
Apparently, the CI failures on illumos-amd64 are due to DNS delay:
Failed tests:
reject_arp_host_nonexistent : filtertest timeout
reject_ip6_host_nonexistent : filtertest timeout
reject_ip_host_nonexistent : filtertest timeout
reject_rarp_host_nonexistent : filtertest timeout
The host occasionally takes more than one second to fail resolving host.invalid
. This reproduces using the current master branch and is not specific to the proposed changes. This pull request is going to be merged tomorrow.
@infrastation
infrastation
merged commit Oct 16, 2025
33574de
into
the-tcpdump-group:master
16 of 17 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes concern a number of similar code paths, which is why it took longer than I expected to enumerate the bugs and to make sure the tests cover the problem space. But here it is.