Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

feature request - Support localhost in address #222

Closed
gaocegege opened this issue Dec 23, 2021 · 4 comments · Fixed by #223
Closed

feature request - Support localhost in address #222

gaocegege opened this issue Dec 23, 2021 · 4 comments · Fixed by #223
Labels
good first issue Good for newcomers

Comments

@gaocegege
Copy link

gaocegege commented Dec 23, 2021

Behavior

$ engula journal run localhost:8888 --mem
error: Invalid value for '<ADDR>': invalid IP address syntax

Expected

Just works as 127.0.0.1:8888

Version

6c59de2

@tisonkun
Copy link
Contributor

It seems Rust's SocketAddr fails to parse "localhost:8888".

            let mut groups = [0; 4];

            for (i, slot) in groups.iter_mut().enumerate() {
                *slot = p.read_separator('.', i, |p| {
                    // Disallow octal number in IP string.
                    // https://tools.ietf.org/html/rfc6943#section-3.1.1
                    p.read_number(10, Some(3), false)
                })?;
            }

            Some(groups.into())

only accept ddd.ddd.ddd.ddd. Thus when Clap tries to parse the string into SocketAddr, it fails with the message above.

@gaocegege
Copy link
Author

Yeah, it is hardcoded to only support IP.

Maybe we should use the struct like inetaddress to support hostname.

@Xuanwo
Copy link

Xuanwo commented Dec 23, 2021

ToSocketAddrs has been implemented for &str, so it's Ok to use "localhost:12345".to_socket_addrs()

Take https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html#examples for reference.

For engula, I prefer to use String in command args.

@huachaohuang huachaohuang added the good first issue Good for newcomers label Dec 23, 2021
@tisonkun
Copy link
Contributor

@Xuanwo thanks for investigation! I learn from how we previous tested grpc implement and prepared a patch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants