Skip to content
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

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

ktmlm
Copy link
Contributor

@ktmlm ktmlm commented Sep 30, 2022

tendermint core can accept unix abstract socket, but this crate can NOT parse it.

this pr fix this issue.

ref:

Comment on lines 91 to 95
if addr.starts_with("unix://@") {
return Ok(Self::Unix {
path: addr.trim_start_matches("unix://").to_owned(),
});
}
Copy link
Collaborator

@tony-iqlusion tony-iqlusion Sep 30, 2022

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:

Suggested change
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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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-commenter
Copy link

Codecov Report

Merging #1205 (3c03b9d) into main (00401ae) will increase coverage by 0.0%.
The diff coverage is 50.0%.

@@          Coverage Diff          @@
##            main   #1205   +/-   ##
=====================================
  Coverage   64.0%   64.0%           
=====================================
  Files        250     250           
  Lines      21554   21560    +6     
=====================================
+ Hits       13798   13805    +7     
+ Misses      7756    7755    -1     
Impacted Files Coverage Δ
config/src/net.rs 90.1% <50.0%> (-2.1%) ⬇️
testgen/src/header.rs 82.9% <0.0%> (-0.6%) ⬇️
tendermint/src/node.rs 63.9% <0.0%> (+0.1%) ⬆️
light-client-verifier/src/types.rs 38.7% <0.0%> (+0.2%) ⬆️
testgen/src/vote.rs 84.8% <0.0%> (+0.8%) ⬆️
tendermint/src/block/commit_sig.rs 80.3% <0.0%> (+1.9%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@thanethomson thanethomson left a 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.

@thanethomson thanethomson merged commit c84a4ff into informalsystems:main Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants