-
Notifications
You must be signed in to change notification settings - Fork 239
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
[discovery-proxy] remove assert
checks on query name parse errors
#2208
[discovery-proxy] remove assert
checks on query name parse errors
#2208
Conversation
assert
checks on query name parse errorsassert
checks on query name parse errors
1e0cd5b
to
9f87be2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
if (splitError != OTBR_ERROR_NONE) | ||
{ | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it may worth a debug or info log for triage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be a good idea but may also extra logs (if there are many queries with invalid format).
I leave it to you or other folks to add logs in future PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that if the client is doing a resolve, but we get a service published at the time, the path of:
case OT_DNSSD_QUERY_TYPE_BROWSE:
splitError = SplitFullServiceName(queryName, serviceName, domain);
will get hit, but that will (and should) always fail since the request was for an instance, not a service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2208 +/- ##
===========================================
- Coverage 55.77% 39.52% -16.26%
===========================================
Files 87 88 +1
Lines 6890 9705 +2815
Branches 0 712 +712
===========================================
- Hits 3843 3836 -7
- Misses 3047 5675 +2628
- Partials 0 194 +194 ☔ View full report in Codecov by Sentry. |
Remove the `assert` checks on query name parse errors and instead skip over such entries. Query names are external input (received as DNS query message from other devices) and discovery proxy code should not assume that external input will follow the proper query name format.
9f87be2
to
1d43f50
Compare
Remove the
assert
checks on query name parse errors and instead skip over such entries. Query names are external input (received as DNS query message from other devices) and discovery proxy code should not assume that external input will follow the proper query name format.