-
Notifications
You must be signed in to change notification settings - Fork 228
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
support parsing unix abstract socket
#1205
support parsing unix abstract socket
#1205
Conversation
config/src/net.rs
Outdated
if addr.starts_with("unix://@") { | ||
return Ok(Self::Unix { | ||
path: addr.trim_start_matches("unix://").to_owned(), | ||
}); | ||
} |
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.
It might make sense to file this logic below with the rest of the code that handles the unix://
prefix.
However, if you want to keep it like this, I'd suggest using strip_prefix
:
if addr.starts_with("unix://@") { | |
return Ok(Self::Unix { | |
path: addr.trim_start_matches("unix://").to_owned(), | |
}); | |
} | |
if let Some(path) = addr.strip_prefix("unix://@") { | |
return Ok(Self::Unix { | |
path: path.to_owned(), | |
}); | |
} |
Edit: I now see this doesn't properly handle the @
, so I guess that doesn't work.
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.
yes, the '@' should be kept, your suggestions can not run.
'moving this code block to the rest of the code that handles the unix://
prefix', this is a good idea, but a unix://@...
is very likely to be denied by the following Url::parse
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 change I commit are the only one case that get tested in my practice
Codecov Report
@@ Coverage Diff @@
## main #1205 +/- ##
=====================================
Coverage 64.0% 64.0%
=====================================
Files 250 250
Lines 21554 21560 +6
=====================================
+ Hits 13798 13805 +7
+ Misses 7756 7755 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Seems reasonable to me. I'd be very interested to know who actually uses abstract Unix domain sockets with Tendermint.
tendermint core can accept
unix abstract socket
, but this crate can NOT parse it.this pr fix this issue.
ref:
man 7 unix
on Linux