Attention is currently required from: dexter, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email )
Change subject: mgcp-cli: Transmit remote IP addr in CRCX if known and port=0
......................................................................
Patch Set 3:
(1 comment)
Patchset:
PS3:
do you have a test for it?
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/35152?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I30165dbac5e484011d0acf46af36f105954a501d
Gerrit-Change-Number: 35152
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 30 Nov 2023 23:58:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35180?usp=email )
Change subject: socket: Introduce API osmo_sock_multiaddr_get_name_buf()
......................................................................
Patch Set 1:
(5 comments)
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/2ad85b55_eccf6410
PS1, Line 1968: return -EBADF;
i understand, but for the snprintf() like function signature, if at all possible you should always return sb.chars_needed. Otherwise, if anyone calls OSMO_STRBUF_APPEND(sb, osmo_sock_multiaddr_get_name_buf, ...), they don't get "<error-bad-fd>" returned, in the expected place; instead, it looks like OSMO_STRBUF_APPEND() might even crash? [the place handling < 0 there looks a bit weird; same as insufficient buffer, which should be a different case.]
the function's purpose is to print a string, not to validate an fd, so better just do this here:
OSMO_STRBUF_PRINTF(sb, "<error-bad-fd>");
return sb.chars_needed;
because then the caller will get the error output in the right place, like, contrived example:
Stats:
reads: 0
writes: 0
address: <error-bad-fd>
When you return an error, the string composition may abort and omit the "Stats:..." part too in the resulting output.
(as i read on, i see that osmo_sock_get_name_buf() makes the same mistake, i guess i need to fix that)
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/64fdb01a_d46bb32b
PS1, Line 1993: v6 = !!s
assigning a value to is_v6 in an 'else' clause and using is_v6 below. It looks like the implication is that if num_hostbuf <= 1, then it cannot be a v6 address, which sounds odd to me
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/08a76724_bd72f529
PS1, Line 2005: OSMO_STRBUF_PRINTF(sb, "<need-more-bufs!>");
(could print this above instead of adding the need_more_bufs local var, just my opinion)
(also i find it weird to print <need-more-bufs!> in user output, it makes no sense to the reader, because it is reporting an error in internal buffer sizes, and not about the user's config or current status of resources; a user may think that a RAM upgrade is necessary to run osmocom, wildly wrong)
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/accd4083_8c902ac9
PS1, Line 2019: bool is_v6 = false;
(can this code dup be collapsed with the above, maybe with a static func?)
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/0dcd0bb5_dbc45421
PS1, Line 2052: int osmo_sock_get_name_buf(char *str, size_t str_len, int fd)
> Something which we could do is internally integrating the multiaddr support into the existing genera […]
my first thought is "yes, sounds good".
Do you think though there may be situations where we don't want all of the addresses in, say, all of the logging or vty output all the time, where instead we want to just continue printing the first address?
I guess that is hard to answer, so unfortunately the best backwards compatible way is to only have the separate new API returning multiple addrs. Callers can switch to the new function at their own time.
But, hey, how about we call the new function osmo_sock_get_name_buf2(), as in, v2 supports multiple addresses, and everyone should just use that everywhere from now on?
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35180?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I48950754ed6f61ee5ffa04a447fab8903f10acc0
Gerrit-Change-Number: 35180
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 30 Nov 2023 23:39:27 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: laforge, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35171?usp=email )
Change subject: soft_uart: check Rx/Tx state once in osmo_soft_uart_{rx,tx}_ubits()
......................................................................
Patch Set 3:
(2 comments)
Patchset:
PS3:
> Btw, -EAGAIN doesn't sound like the best error code to return in that situation. […]
The idea here is to indicate the caller that for whatever reason (here it's "the Rx/Tx is disabled", in the next patch it would also be "Rx/TX is suspended by flow control") the receiver/transmitter did not consume/emit the requested amount of bits. And `EAGAIN` should be interpreted as "try next time, things may change".
The caller of `osmo_soft_uart_{rx,tx}_ubits()` is expected not to be in charge of the soft-UART management (enabling or disabling Rx/Tx lines). In a typical scenario it's a logic driving some synchronous medium (like a TCH channel, on which you always Rx and always Tx something).
* If the `_rx_ubits()` API returns `-EAGAIN` (or any other `N != 0`), the caller is likely to ignore this and keep feeding the UART with bits received over the synchronous medium.
* If the `_tx_ubits()` API returns `-EAGAIN` (or any other `N != 0`), it's responsibility of the caller to initialize the unpopulated bit buffer with stop bits (binary '1') and transmit them over the synchronous medium.
I am open for suggestions if you have a better errno value in mind ;)
File src/core/soft_uart.c:
https://gerrit.osmocom.org/c/libosmocore/+/35171/comment/47f53bd2_09d4bf26
PS3, Line 291: if (!suart->tx.running)
> Move this above the other if condition, this one one can use tx_ubits(... […]
I would better go for adding proper API, if there would be a need for checking Rx/Tx running state. `-EAGAIN` would also be returned if the Rx/Tx is suspended due to flow control (see the next patch), so... no. Let's keep it as-is.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35171?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I70f93b3655eb21c2323e451052c40cd305c016c8
Gerrit-Change-Number: 35171
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 30 Nov 2023 20:39:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35180?usp=email )
Change subject: socket: Introduce API osmo_sock_multiaddr_get_name_buf()
......................................................................
Patch Set 1:
(1 comment)
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35180/comment/2e83f24a_61d0386d
PS1, Line 2052: int osmo_sock_get_name_buf(char *str, size_t str_len, int fd)
Something which we could do is internally integrating the multiaddr support into the existing general osmo_sock_get_name_buf(), by means of doing inside here:
int sock_type;
socklen_t sock_type_length = sizeof(sock_type);
getsockopt(socket, SOL_SOCKET, SO_TYPE, &sock_type, &sock_type_length);
if (sock_type == IPPROTO_SCTP)
osmo_sock_multiaddr_get_name_buf();
I'm fine with both approaches, the existing one or the one presented in this commit.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35180?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I48950754ed6f61ee5ffa04a447fab8903f10acc0
Gerrit-Change-Number: 35180
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-CC: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 30 Nov 2023 19:16:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35179?usp=email )
Change subject: socket: Introduce API osmo_sock_multiaddr_get_ip_and_port()
......................................................................
socket: Introduce API osmo_sock_multiaddr_get_ip_and_port()
Related: SYS#6636
Related: OS#5581
Change-Id: I19d560ab4aadec18a4c0f94115675ec1d7ab14d7
---
M TODO-RELEASE
M include/osmocom/core/socket.h
M src/core/libosmocore.map
M src/core/socket.c
4 files changed, 94 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/79/35179/1
diff --git a/TODO-RELEASE b/TODO-RELEASE
index fa7bc57..e365746 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -8,6 +8,7 @@
# If any interfaces have been removed or changed since the last public release: c:r:0.
#library what description / commit summary line
core ADD osmo_sock_multiaddr_{add,del}_local_addr()
+core ADD osmo_sock_multiaddr_get_ip_and_port()
core ADD gsmtap_inst_fd2() core, DEPRECATE gsmtap_inst_fd()
isdn ABI change add states and flags for external T200 handling
gsm ABI change add T200 timer states to lapdm_datalink
diff --git a/include/osmocom/core/socket.h b/include/osmocom/core/socket.h
index 03f1d39..9397343 100644
--- a/include/osmocom/core/socket.h
+++ b/include/osmocom/core/socket.h
@@ -191,6 +191,8 @@
int osmo_sock_get_remote_ip(int fd, char *host, size_t len);
int osmo_sock_get_remote_ip_port(int fd, char *port, size_t len);
+int osmo_sock_multiaddr_get_ip_and_port(int fd, int ip_proto, char *ip, size_t ip_len, size_t *ip_cnt,
+ char *port, size_t port_len, bool local);
int osmo_sock_multiaddr_add_local_addr(int sfd, const char **addrs, size_t addrs_cnt);
int osmo_sock_multiaddr_del_local_addr(int sfd, const char **addrs, size_t addrs_cnt);
diff --git a/src/core/libosmocore.map b/src/core/libosmocore.map
index 3d6aa42..1d00fe8 100644
--- a/src/core/libosmocore.map
+++ b/src/core/libosmocore.map
@@ -436,6 +436,7 @@
osmo_sock_mcast_ttl_set;
osmo_sock_multiaddr_add_local_addr;
osmo_sock_multiaddr_del_local_addr;
+osmo_sock_multiaddr_get_ip_and_port;
osmo_sock_set_dscp;
osmo_sock_set_priority;
osmo_sock_unix_init;
diff --git a/src/core/socket.c b/src/core/socket.c
index 1dc8e46..1d97018 100644
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -1807,6 +1807,85 @@
return 0;
}
+/*! Get the IP and/or port number on socket in separate string buffers.
+ * \param[in] fd file descriptor of socket
+* \param[out] ip_proto IPPROTO of the socket, eg: IPPROTO_SCTP.
+ * \param[out] ip Pointer to memory holding consecutive buffers of size ip_len
+ * \param[in] ip_len length of each of the string buffer in the the ip array
+ * \param[in] ip_cnt length ip array pointer. on return it contains the number of addresses found.
+ * \param[out] port number (will be filled in when not NULL)
+ * \param[in] port_len length of the port buffer
+ * \param[in] local (true) or remote (false) name will get looked at
+ * \returns 0 on success; negative otherwise
+ *
+ * Upon return, ip_cnt can be set to a higher value than the one set by the
+ * caller. This can be used by the caller to find out the required array length
+ * and then obtaining by calling the function twice. Only up to ip_cnt addresses
+ * are filed in, as per the value provided by the caller.
+ */
+int osmo_sock_multiaddr_get_ip_and_port(int fd, int ip_proto, char *ip, size_t ip_len, size_t *ip_cnt,
+ char *port, size_t port_len, bool local)
+{
+ struct sockaddr *addrs = NULL;
+ unsigned int n_addrs, i;
+ void *addr_buf;
+ int rc;
+
+ switch (ip_proto) {
+ case IPPROTO_SCTP:
+ break; /* continue below */
+ default:
+ if (*ip_cnt == 0) {
+ *ip_cnt = 1;
+ return 0;
+ }
+ *ip_cnt = 1;
+ rc = osmo_sock_get_ip_and_port(fd, ip, ip_len, port, port_len, local);
+ return rc;
+ }
+
+ rc = local ? sctp_getladdrs(fd, 0, &addrs) : sctp_getpaddrs(fd, 0, &addrs);
+ if (rc < 0)
+ return rc;
+ if (rc == 0)
+ return -ENOTCONN;
+
+ n_addrs = rc;
+ addr_buf = (void *)addrs;
+ for (i = 0; i < n_addrs; i++) {
+ struct sockaddr *sa_addr = (struct sockaddr *)addr_buf;
+ size_t addrlen;
+
+ if (i >= *ip_cnt)
+ break;
+
+ switch (sa_addr->sa_family) {
+ case AF_INET:
+ addrlen = sizeof(struct sockaddr_in);
+ break;
+ case AF_INET6:
+ addrlen = sizeof(struct sockaddr_in6);
+ break;
+ default:
+ local ? sctp_freeladdrs(addrs) : sctp_freepaddrs(addrs);
+ return -EINVAL;
+ }
+
+ rc = getnameinfo(sa_addr, addrlen, &ip[i*ip_len], ip_len,
+ port, port_len,
+ NI_NUMERICHOST | NI_NUMERICSERV);
+ if (rc < 0) {
+ local ? sctp_freeladdrs(addrs) : sctp_freepaddrs(addrs);
+ return rc;
+ }
+ addr_buf += addrlen;
+ }
+
+ *ip_cnt = n_addrs;
+ local ? sctp_freeladdrs(addrs) : sctp_freepaddrs(addrs);
+ return 0;
+}
+
/*! Get local IP address on socket
* \param[in] fd file descriptor of socket
* \param[out] ip IP address (will be filled in)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35179?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I19d560ab4aadec18a4c0f94115675ec1d7ab14d7
Gerrit-Change-Number: 35179
Gerrit-PatchSet: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: jolly, laforge.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33532?usp=email )
Change subject: mobile: Fix PCS ARFCN handling: PCS can only be ARFCN 512..810
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS4:
The `uint8_t` to `bool` change makes this patch harder to read, ideally this should be done separately . Other than that, looks good to me.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33532?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Id99c8534bf853f4f24f99364790c1ac1df6cc007
Gerrit-Change-Number: 33532
Gerrit-PatchSet: 4
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 30 Nov 2023 18:00:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment