On 2/1/23 15:52, Frode Nordahl wrote:
> Commit 08e9e5337383 fixed proper initialization of the dns-resolve
> module, and made DNS resolution asynchronous.
>
> A side effect of that change revealed a long standing logic bug
> which broke ovsdb-server listener configuration using DNS names.
>
> Previously this worked because the DNS resolution would block,
> now that DNS resolution is asynchronous the code before this
> change would assume the error from jsonrpc_pstream_open meant
> the listener was in fact a specification for an active
> outgoing connection, even when that was not the case.
>
> To fix this a couple of changes was made to socket-util:
> 1) Pass optional result of dns resolution from inet_parse_passive.
>
> When (re-)configuring listeners that use DNS names, we may need
> to know whether the provided connection string is invalid or if
> the provided DNS name has finished resolving.
>
> 2) Check dns resolution status in inet_open_passive.
>
> If the connection string is valid, and contains a DNS name,
> inet_open_passive will now return -ENODATA if dns resolution
> failed. DNS resolution failure may either mean the asynchronous
> resolver has not completed yet, or that the name does not resolve.
>
> Reported-at: https://bugs.launchpad.net/bugs/1998781
> Fixes: 08e9e5337383 ("ovsdb: raft: Fix inability to read the database with
> DNS host names.")
> Fixes: 771680d96fb6 ("DNS: Add basic support for asynchronous DNS resolving")
> Signed-off-by: Frode Nordahl <[email protected]>
> ---
> lib/socket-util.c | 13 ++++++++++---
> lib/socket-util.h | 3 ++-
> ovsdb/jsonrpc-server.c | 43 ++++++++++++++++++++++++++----------------
> 3 files changed, 39 insertions(+), 20 deletions(-)
Hi, Frode. Thanks for v2.
I was trying to run tests with this patch and, apparently, FreeBSD
doesn't have ENODATA:
lib/socket-util.c:720:21: error: 'ENODATA' undeclared (first use in this
function); did you mean 'NO_DATA'?
720 | return -ENODATA;
| ^~~~~~~
| NO_DATA
We should, probably, use some other errno here.
Best regards, Ilya Maximets.
>
> diff --git a/lib/socket-util.c b/lib/socket-util.c
> index 38705cc51..eba350ccb 100644
> --- a/lib/socket-util.c
> +++ b/lib/socket-util.c
> @@ -660,7 +660,8 @@ exit:
> * zeros '*ss' and returns false. */
> bool
> inet_parse_passive(const char *target_, int default_port,
> - struct sockaddr_storage *ss)
> + struct sockaddr_storage *ss,
> + bool resolve_host, bool *dns_failure)
> {
> char *target = xstrdup(target_);
> char *port, *host;
> @@ -672,7 +673,7 @@ inet_parse_passive(const char *target_, int default_port,
> ok = false;
> } else {
> ok = parse_sockaddr_components(ss, host, port, default_port,
> - target_, true, NULL);
> + target_, resolve_host, dns_failure);
> }
> if (!ok) {
> memset(ss, 0, sizeof *ss);
> @@ -710,8 +711,14 @@ inet_open_passive(int style, const char *target, int
> default_port,
> struct sockaddr_storage ss;
> int fd = 0, error;
> unsigned int yes = 1;
> + bool dns_failure;
>
> - if (!inet_parse_passive(target, default_port, &ss)) {
> + if (!inet_parse_passive(target, default_port, &ss, true, &dns_failure)) {
> + if (dns_failure) {
> + /* DNS failure means asynchronous DNS resolution is in progress,
> + * or that the name does currently not resolve. */
> + return -ENODATA;
> + }
> return -EAFNOSUPPORT;
> }
> kernel_chooses_port = ss_get_port(&ss) == 0;
> diff --git a/lib/socket-util.h b/lib/socket-util.h
> index bf66393df..4eec627e3 100644
> --- a/lib/socket-util.h
> +++ b/lib/socket-util.h
> @@ -55,7 +55,8 @@ int inet_open_active(int style, const char *target, int
> default_port,
> struct sockaddr_storage *ssp, int *fdp, uint8_t dscp);
>
> bool inet_parse_passive(const char *target, int default_port,
> - struct sockaddr_storage *ssp);
> + struct sockaddr_storage *ssp,
> + bool resolve_host, bool *dns_failure);
> int inet_open_passive(int style, const char *target, int default_port,
> struct sockaddr_storage *ssp, uint8_t dscp,
> bool kernel_print_port);
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 916a1f414..b4eb17467 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -267,25 +267,36 @@ ovsdb_jsonrpc_server_add_remote(struct
> ovsdb_jsonrpc_server *svr,
> int error;
>
> error = jsonrpc_pstream_open(name, &listener, options->dscp);
> - if (error && error != EAFNOSUPPORT) {
> - VLOG_ERR_RL(&rl, "%s: listen failed: %s", name, ovs_strerror(error));
> - return NULL;
> - }
> + switch (error) {
> + case 0:
> + case EAFNOSUPPORT:
> + remote = xmalloc(sizeof *remote);
> + remote->server = svr;
> + remote->listener = listener;
> + ovs_list_init(&remote->sessions);
> + remote->dscp = options->dscp;
> + remote->read_only = options->read_only;
> + remote->role = nullable_xstrdup(options->role);
> + shash_add(&svr->remotes, name, remote);
> + if (!listener) {
> + /* Not a listener, attempt creation of active jsonrpc sesssion.
> */
> + ovsdb_jsonrpc_session_create(remote,
> + jsonrpc_session_open(name, true),
> + svr->read_only ||
> remote->read_only);
> + }
> + return remote;
>
> - remote = xmalloc(sizeof *remote);
> - remote->server = svr;
> - remote->listener = listener;
> - ovs_list_init(&remote->sessions);
> - remote->dscp = options->dscp;
> - remote->read_only = options->read_only;
> - remote->role = nullable_xstrdup(options->role);
> - shash_add(&svr->remotes, name, remote);
> + case ENODATA:
> + VLOG_DBG_RL(&rl, "%s: listen failed: "
> + "DNS resolution in progress or host not found",
> name);
> + return NULL;
>
> - if (!listener) {
> - ovsdb_jsonrpc_session_create(remote, jsonrpc_session_open(name,
> true),
> - svr->read_only || remote->read_only);
> + default:
> + VLOG_ERR_RL(&rl, "%s: listen failed: %s", name,
> + ovs_strerror(error));
> + return NULL;
> }
> - return remote;
> + OVS_NOT_REACHED();
> }
>
> static void
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
