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
Fix dohpath parsing/stringify #1366
Conversation
svcb.go
Outdated
s.Template = b | ||
template, err := svcbParseParam(b) | ||
if err != nil { | ||
return fmt.Errorf("dns: svcblocal: svcb private/experimental key %w", err) |
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.
This message should probably be about svcbdohpath
?
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.
Whoops! Cut & paste error. I've fix this, thanks for spotting!
@shane-kerr probably makes sense to rework #1363 to make that more generic to handle all this stuff with 1 or 2 functions? Or does this obsolete that PR? |
@miekg when I looked at #1363 I considered using a single function for parsing the alpn and the local stuff, but the encoded comma-separated list means that the rules are actually quite different. On my first attempt, I used the This PR is different, since the handling for parsing and stringify is actually identical for the dohpath and the local stuff. I'm pretty sure the two PR don't collide, so can both be merged usefully. If we really want to refactor code to remove redundancy, we could look at pulling out the escaping functionality in |
I'll hold off merging, until #1363 is in, might result in conflicts otherwise? |
Yes, makes sense. I don't expect conflicts, but we'll see! |
* dohpath escaped in String(), and parsed such values * Update the test for dohpath with escaping * Fix cut & paste error with svcdohpath error
I noticed that the brand-new dohpath support has a similar problem to the one addressed in PR #1361 - non-printable characters are not handled properly.
Luckily the parameter is much simpler than the alpn parameter. I refactored the parse/stringify
SVCBLocal
out and use that for both types.