WIP: PR for allowing hostnames in config files #42
Conversation
|
@TSC21 Can you tag anyone that you think may be able to help on this PR, please? Thanks! |
| @@ -797,14 +797,9 @@ static int parse_confs(ConfFile &conf) | |||
| log_error("Expected 'port' key for section %.*s", (int)iter.name_len, iter.name); | |||
| ret = -EINVAL; | |||
| } else { | |||
| if (validate_ip(opt_udp.addr) < 0) { | |||
There was a problem hiding this comment.
What happens now if the ip/hostname is invalid? Does it crash somewhere, or does it just not work? Wouldn't it be nicer to have an error message similar to the one removed here?
There was a problem hiding this comment.
Agree that it would be better to keep the validation check, but it looks like the check for valid hostnames can be quite extensive per RFC1123; maybe better to defer it to the hostname lookup stage?
There was a problem hiding this comment.
But then won't it fail in the getaddrinfo call (line 1169)? Shouldn't it print an error message (some log_error()) there? Right now it feels like it just does exit(1), so we won't know what happened from the logs, right? Or did I miss something?
| #ifdef ENABLE_IPV6 | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Does that compile when ENABLE_IPV6 is enabled? Feels like you removed the opening bracket 🤔
| @@ -797,14 +797,9 @@ static int parse_confs(ConfFile &conf) | |||
| log_error("Expected 'port' key for section %.*s", (int)iter.name_len, iter.name); | |||
| ret = -EINVAL; | |||
| } else { | |||
| if (validate_ip(opt_udp.addr) < 0) { | |||
There was a problem hiding this comment.
But then won't it fail in the getaddrinfo call (line 1169)? Shouldn't it print an error message (some log_error()) there? Right now it feels like it just does exit(1), so we won't know what happened from the logs, right? Or did I miss something?
| // #ifdef ENABLE_IPV6 | ||
| // if (this->is_ipv6) { | ||
| // if (connect(fd, (struct sockaddr *)&sockaddr6, sizeof(sockaddr6)) < 0) { | ||
| // log_error("Error connecting to IPv6 socket (%m)"); | ||
| // goto fail; | ||
| // } | ||
| // } else { | ||
| // #endif | ||
| // if (connect(fd, addrs[0]->ai_addr, addrs[0]->ai_addrlen) < 0) { | ||
| // log_error("Error connecting to socket (%m)"); | ||
| // goto fail; | ||
| // } | ||
| // #ifdef ENABLE_IPV6 | ||
| // } | ||
| // #endif |
There was a problem hiding this comment.
| // #ifdef ENABLE_IPV6 | |
| // if (this->is_ipv6) { | |
| // if (connect(fd, (struct sockaddr *)&sockaddr6, sizeof(sockaddr6)) < 0) { | |
| // log_error("Error connecting to IPv6 socket (%m)"); | |
| // goto fail; | |
| // } | |
| // } else { | |
| // #endif | |
| // if (connect(fd, addrs[0]->ai_addr, addrs[0]->ai_addrlen) < 0) { | |
| // log_error("Error connecting to socket (%m)"); | |
| // goto fail; | |
| // } | |
| // #ifdef ENABLE_IPV6 | |
| // } | |
| // #endif |
|
|
||
| struct addrinfo hints = {0}, *addrs; | ||
| hints.ai_family = AF_UNSPEC; | ||
| hints.ai_socktype = SOCK_STREAM; |
There was a problem hiding this comment.
For a UdpEndpoint, shouldn't that be a SOCK_DGRAM?
Objective
To allow specification of hostnames (rather than just IP addresses) in mavlink-router config files.
Context
It seems as if this feature isn't part of the upstream mavlink-router yet, either, but we're looking to be able to specify a hostname that can be resolved dynamically at runtime.
The goal would be to allow configurations such as the following:
The current result when using this config is:
Invalid IP address in section UdpEndpoint qgroundcontrol: qgc.auterion.comCurrent Status
Note that this initial patch is a WIP (i.e., it does not seem to add the feature properly); it was my best stab at making it work. @TSC21 suggested I open a PR to get some feedback/help. Thanks!