On Tue, Feb 7, 2023 at 3:46 PM Ilya Maximets <[email protected]> wrote:
>
> 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.
I forgot about our non-Linux friends, I'll find something else that
fits and send a v3. Thank you for catching it, and sorry for not
checking this.
--
Frode Nordahl
> 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
