Change in libosmo-netif[master]: Deprecate osmo_stream_cli_open2()

Harald Welte gerrit-no-reply at lists.osmocom.org
Tue Mar 19 13:40:55 UTC 2019


Harald Welte has submitted this change and it was merged. ( https://gerrit.osmocom.org/12845 )

Change subject: Deprecate osmo_stream_cli_open2()
......................................................................

Deprecate osmo_stream_cli_open2()

This supposed to be variant of osmo_stream_cli_open() with explicit
control over reconnection logic but it's plain broken: doxygen docs
contradict the code, actual reconnection logic is affected by timeout
parameter directly which is set in different function.

It seems like we haven't been affected by this so far because we always
use it in auto-reconnection mode which is triggered by default due to
positive reconnection timeout value (5 sec) automatically used in the
absense of explicitly set timeout.

Looking at commit history, this function already been source of
confusion in the past. Instead of trying to fix this mess, let's just
deprecate it entirely and properly document use of
osmo_stream_cli_set_reconnect_timeout() to control reconnection logic.

The only known user is libosmo-sccp which won't use it as of
0a93a683f3cb8e5977eb4a666ab207db6e7d7af9 commit.

Change-Id: Id988ed0274b363db049f59cbf6a193727c8c3c8a
---
M include/osmocom/netif/stream.h
M src/stream.c
M tests/stream/stream_test.c
3 files changed, 120 insertions(+), 7 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  osmith: Looks good to me, but someone else must approve
  Jenkins Builder: Verified



diff --git a/include/osmocom/netif/stream.h b/include/osmocom/netif/stream.h
index 56162e4..f1c160c 100644
--- a/include/osmocom/netif/stream.h
+++ b/include/osmocom/netif/stream.h
@@ -72,7 +72,8 @@
 void osmo_stream_cli_destroy(struct osmo_stream_cli *cli);
 
 int osmo_stream_cli_open(struct osmo_stream_cli *cli);
-int osmo_stream_cli_open2(struct osmo_stream_cli *cli, int reconnect);
+int osmo_stream_cli_open2(struct osmo_stream_cli *cli, int reconnect) \
+	OSMO_DEPRECATED("Use osmo_stream_cli_set_reconnect_timeout() or osmo_stream_cli_reconnect() instead");
 void osmo_stream_cli_close(struct osmo_stream_cli *cli);
 
 void osmo_stream_cli_send(struct osmo_stream_cli *cli, struct msgb *msg);
diff --git a/src/stream.c b/src/stream.c
index c4db3d7..3d0b665 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -166,8 +166,9 @@
 void osmo_stream_cli_close(struct osmo_stream_cli *cli);
 
 /*! \brief Re-connect an Osmocom Stream Client
- *  If re-connection is enabled for this client, we close any existing
- *  connection (if any) and schedule a re-connect timer */
+ *  If re-connection is enabled for this client
+ *  (which is the case unless negative timeout was explicitly set via osmo_stream_cli_set_reconnect_timeout() call),
+ *  we close any existing connection (if any) and schedule a re-connect timer */
 void osmo_stream_cli_reconnect(struct osmo_stream_cli *cli)
 {
 	osmo_stream_cli_close(cli);
@@ -391,7 +392,7 @@
 
 /*! \brief Set the reconnect time of the stream client socket
  *  \param[in] cli Stream Client to modify
- *  \param[in] timeout Re-connect timeout in seconds */
+ *  \param[in] timeout Re-connect timeout in seconds or negative value to disable auto-reconnection */
 void
 osmo_stream_cli_set_reconnect_timeout(struct osmo_stream_cli *cli, int timeout)
 {
@@ -475,7 +476,8 @@
 	talloc_free(cli);
 }
 
-/*! \brief Open connection of an Osmocom stream client
+/*! \brief DEPRECATED: use osmo_stream_cli_set_reconnect_timeout() or osmo_stream_cli_reconnect() instead!
+ * Open connection of an Osmocom stream client
  *  \param[in] cli Stream Client to connect
  *  \param[in] reconect 1 if we should not automatically reconnect
  */
@@ -534,10 +536,44 @@
 }
 
 /*! \brief Open connection of an Osmocom stream client
+ *  By default the client will automatically reconnect after default timeout.
+ *  To disable this, use osmo_stream_cli_set_reconnect_timeout() before calling this function.
  *  \param[in] cli Stream Client to connect */
 int osmo_stream_cli_open(struct osmo_stream_cli *cli)
 {
-	return osmo_stream_cli_open2(cli, 0);
+	int ret;
+
+	/* we are reconfiguring this socket, close existing first. */
+	if ((cli->flags & OSMO_STREAM_CLI_F_RECONF) && cli->ofd.fd >= 0)
+		osmo_stream_cli_close(cli);
+
+	cli->flags &= ~OSMO_STREAM_CLI_F_RECONF;
+
+	ret = osmo_sock_init2(AF_INET, SOCK_STREAM, cli->proto,
+			      cli->local_addr, cli->local_port,
+			      cli->addr, cli->port,
+			      OSMO_SOCK_F_CONNECT|OSMO_SOCK_F_BIND|OSMO_SOCK_F_NONBLOCK);
+	if (ret < 0) {
+		osmo_stream_cli_reconnect(cli);
+		return ret;
+	}
+	cli->ofd.fd = ret;
+
+	if (cli->flags & OSMO_STREAM_CLI_F_NODELAY) {
+		ret = setsockopt_nodelay(cli->ofd.fd, cli->proto, 1);
+		if (ret < 0)
+			goto error_close_socket;
+	}
+
+	if (osmo_fd_register(&cli->ofd) < 0)
+		goto error_close_socket;
+
+	return 0;
+
+error_close_socket:
+	close(ret);
+	cli->ofd.fd = -1;
+	return -EIO;
 }
 
 static void cli_timer_cb(void *data)
@@ -549,7 +585,7 @@
 	switch(cli->state) {
 	case STREAM_CLI_STATE_CONNECTING:
 		cli->ofd.when |= BSC_FD_READ | BSC_FD_WRITE;
-		osmo_stream_cli_open2(cli, 1);
+		osmo_stream_cli_open(cli);
 	        break;
 	default:
 		break;
diff --git a/tests/stream/stream_test.c b/tests/stream/stream_test.c
index 1a0c555..439ea1a 100644
--- a/tests/stream/stream_test.c
+++ b/tests/stream/stream_test.c
@@ -126,6 +126,76 @@
 	return cli;
 }
 
+/* Without explicit timeout set with osmo_stream_cli_set_reconnect_timeout() default value is used.
+static struct osmo_stream_cli *init_client_reconnection_broken1(struct osmo_stream_cli *cli, bool autoreconnect)
+{
+	if (osmo_stream_cli_open2(cli, autoreconnect) < 0) {
+		LOGCLI(cli, "unable to open client\n");
+		return NULL;
+	}
+
+	return cli;
+}
+That's why those those functions result in exact the same output despite inverse use of autoreconnect parameter.
+static struct osmo_stream_cli *init_client_reconnection_broken2(struct osmo_stream_cli *cli, bool autoreconnect)
+{
+	if (osmo_stream_cli_open2(cli, !autoreconnect) < 0) {
+		LOGCLI(cli, "unable to open client\n");
+		return NULL;
+	}
+
+	return cli;
+}
+
+Variant below are also equivalent to each other.
+static struct osmo_stream_cli *init_client_reconnection_broken1(struct osmo_stream_cli *cli, bool autoreconnect)
+{
+	osmo_stream_cli_set_reconnect_timeout(cli, (!autoreconnect) ? 2 : -1);
+	if (osmo_stream_cli_open2(cli, autoreconnect) < 0) {
+		LOGCLI(cli, "unable to open client\n");
+		return NULL;
+	}
+
+	return cli;
+}
+
+static struct osmo_stream_cli *init_client_reconnection_broken2(struct osmo_stream_cli *cli, bool autoreconnect)
+{
+	osmo_stream_cli_set_reconnect_timeout(cli, (!autoreconnect) ? 2 : -1);
+	if (osmo_stream_cli_open2(cli, !autoreconnect) < 0) {
+		LOGCLI(cli, "unable to open client\n");
+		return NULL;
+	}
+
+	return cli;
+}
+Note: the result differs from normal init_client_reconnection()
+*/
+
+/* Setting reconnection value explicitly as follows is equivalent to normal init_client_reconnection()
+static struct osmo_stream_cli *init_client_reconnection_broken1(struct osmo_stream_cli *cli, bool autoreconnect)
+{
+	osmo_stream_cli_set_reconnect_timeout(cli, autoreconnect ? 2 : -1);
+	if (osmo_stream_cli_open2(cli, autoreconnect) < 0) {
+		LOGCLI(cli, "unable to open client\n");
+		return NULL;
+	}
+
+	return cli;
+}
+
+static struct osmo_stream_cli *init_client_reconnection_broken2(struct osmo_stream_cli *cli, bool autoreconnect)
+{
+	osmo_stream_cli_set_reconnect_timeout(cli, autoreconnect ? 2 : -1);
+	if (osmo_stream_cli_open2(cli, !autoreconnect) < 0) {
+		LOGCLI(cli, "unable to open client\n");
+		return NULL;
+	}
+
+	return cli;
+}
+*/
+
 static struct osmo_stream_cli *make_client(void *ctx, const char *host, unsigned port, bool autoreconnect)
 {
 	struct osmo_stream_cli *cli = osmo_stream_cli_create(ctx);
@@ -141,6 +211,12 @@
 	osmo_stream_cli_set_connect_cb(cli, connect_cb_cli);
 	osmo_stream_cli_set_read_cb(cli, read_cb_cli);
 
+	/* using
+	   return init_client_reconnection_broken1(cli, autoreconnect);
+	   or
+	   return init_client_reconnection_broken2(cli, autoreconnect);
+	   will result in exactly the same output which might or might not be the same as with
+	   init_client_reconnection() - see preceeding notes */
 	return init_client_reconnection(cli, autoreconnect);
 }
 

-- 
To view, visit https://gerrit.osmocom.org/12845
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-netif
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id988ed0274b363db049f59cbf6a193727c8c3c8a
Gerrit-Change-Number: 12845
Gerrit-PatchSet: 8
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190319/447d4632/attachment.html>


More information about the gerrit-log mailing list