Change in libosmocore[master]: add to osmo_sock_get_name*() API
Neels Hofmeyr has submitted this change and it was merged. (
https://gerrit.osmocom.org/12268 )
Change subject: add to osmo_sock_get_name*() API
..
add to osmo_sock_get_name*() API
Basically, I am applying code review that I would have given had I not been on
vacation when the last osmo_sock_get_name* stuff was merged.
osmo_sock_get_name2() is so far a static internal function. However, it is
nothing like osmo_sock_get_name(), so instead rename it to
osmo_sock_get_ip_and_port(). Also make it public API, no need to hide it. I'm
adding an "and" in the name to hopefully clarify: "ip_port" vs. "ip_and_port"
-- there already are _get_X_ip_port() functions that only return the port
string, despite "ip" in the name.
Add new public osmo_sock_get_name2(), which is like osmo_sock_get_name(),
except it uses a static string instead of talloc, and omits the braces. This
is most convenient for log statement formats, avoiding dyn allocations.
Add new osmo_sock_get_name_buf(), which is like osmo_sock_get_name2() but
writes to a caller provided char buffer.
Use osmo_sock_get_name_buf() in the implementation of osmo_sock_get_name(),
but use another (non-static) local string buffer, because adding braces is too
complex without talloc_snprintf().
Rationale:
I want to improve the logging of socket errors, e.g. change
DLMGCP ERROR Failed to read: 111/Connection refused (mgcp_client.c:720)
to
DLMGCP ERROR Failed to read: r=10.0.99.2:2427<->l=10.0.99.2:2728:
111='Connection refused' (mgcp_client.c:721)
but it is just not handy to compose logging with the current API:
- osmo_sock_get_name() requires a talloc_free().
- all the others require output buffers.
- the only way to conveniently compose a logging string and,
- notably, the only trivial way to skip the string composition if the logging
level is currently muted, is to have a function that returns a static string:
the new osmo_sock_get_name2().
- (I think the osmo_sock_get_{local,remote}_* convenience wrappers should never
have been added, because they encourage the caller to invoke the same code
twice, for IP addr and port, and throw away one half each time.)
Related: Iae728192f499330d16836d9435648f6b8ed213b6 (osmo-mgw)
Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398
---
M include/osmocom/core/socket.h
M src/socket.c
2 files changed, 50 insertions(+), 17 deletions(-)
Approvals:
Jenkins Builder: Verified
diff --git a/include/osmocom/core/socket.h b/include/osmocom/core/socket.h
index 79a61bb..43604ec 100644
--- a/include/osmocom/core/socket.h
+++ b/include/osmocom/core/socket.h
@@ -60,6 +60,9 @@
const char *socket_path, unsigned int flags);
char *osmo_sock_get_name(void *ctx, int fd);
+const char *osmo_sock_get_name2(int fd);
+int osmo_sock_get_name_buf(char *str, size_t str_len, int fd);
+int osmo_sock_get_ip_and_port(int fd, char *ip, size_t ip_len, char *port,
size_t port_len, bool local);
int osmo_sock_get_local_ip(int fd, char *host, size_t len);
int osmo_sock_get_local_ip_port(int fd, char *port, size_t len);
int osmo_sock_get_remote_ip(int fd, char *host, size_t len);
diff --git a/src/socket.c b/src/socket.c
index e804ab5..4f3b1ca 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -697,10 +697,7 @@
return osmo_fd_init_ofd(ofd, osmo_sock_unix_init(type, proto,
socket_path, flags));
}
-/*! Get the IP and/or port number on socket. This is for internal usage.
- * Convenience wrappers: osmo_sock_get_local_ip(),
- * osmo_sock_get_local_ip_port(), osmo_sock_get_remote_ip(),
- * osmo_sock_get_remote_ip_port() and osmo_sock_get_name()
+/*! Get the IP and/or port number on socket in separate string buffers.
* \param[in] fd file descriptor of socket
* \param[out] ip IP address (will be filled in when not NULL)
* \param[in] ip_len length of the ip buffer
@@ -709,7 +706,7 @@
* \param[in] local (true) or remote (false) name will get looked at
* \returns 0 on success; negative otherwise
*/
-static int osmo_sock_get_name2(int fd, char *ip, size_t ip_len, char *port,
size_t port_len, bool local)
+int osmo_sock_get_ip_and_port(int fd, char *ip, size_t ip_len, char *port,
size_t port_len, bool local)
{
struct sockaddr sa;
socklen_t len = sizeof(sa);
@@ -741,7 +738,7 @@
*/
int osmo_sock_get_local_ip(int fd, char *ip, size_t len)
{
- return osmo_sock_get_name2(fd, ip, len, NULL, 0, true);
+ return osmo_sock_get_ip_and_port(fd, ip, len, NULL, 0, true);
}
/*! Get local port on socket
@@ -752,7 +749,7 @@
*/
int osmo_sock_get_local_ip_port(int fd, char *port, size_t len)
{
- return osmo_sock_get_name2(fd, NULL, 0, port, len, true);
+ return osmo_sock_get_ip_and_port(fd, NULL, 0, port, len, true);
}
/*! Get remote IP address on socket
@@ -763,7 +760,7 @@
*/
int osmo_sock_get_remote_ip(int fd, char *ip, size_t len)
{
- return osmo_sock_get_name2(fd, ip, len,
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Hello Pau Espin Pedrol, Harald Welte, Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/12268
to look at the new patch set (#3).
Change subject: add to osmo_sock_get_name*() API
..
add to osmo_sock_get_name*() API
Basically, I am applying code review that I would have given had I not been on
vacation when the last osmo_sock_get_name* stuff was merged.
osmo_sock_get_name2() is so far a static internal function. However, it is
nothing like osmo_sock_get_name(), so instead rename it to
osmo_sock_get_ip_and_port(). Also make it public API, no need to hide it. I'm
adding an "and" in the name to hopefully clarify: "ip_port" vs. "ip_and_port"
-- there already are _get_X_ip_port() functions that only return the port
string, despite "ip" in the name.
Add new public osmo_sock_get_name2(), which is like osmo_sock_get_name(),
except it uses a static string instead of talloc, and omits the braces. This
is most convenient for log statement formats, avoiding dyn allocations.
Add new osmo_sock_get_name_buf(), which is like osmo_sock_get_name2() but
writes to a caller provided char buffer.
Use osmo_sock_get_name_buf() in the implementation of osmo_sock_get_name(),
but use another (non-static) local string buffer, because adding braces is too
complex without talloc_snprintf().
Rationale:
I want to improve the logging of socket errors, e.g. change
DLMGCP ERROR Failed to read: 111/Connection refused (mgcp_client.c:720)
to
DLMGCP ERROR Failed to read: r=10.0.99.2:2427<->l=10.0.99.2:2728:
111='Connection refused' (mgcp_client.c:721)
but it is just not handy to compose logging with the current API:
- osmo_sock_get_name() requires a talloc_free().
- all the others require output buffers.
- the only way to conveniently compose a logging string and,
- notably, the only trivial way to skip the string composition if the logging
level is currently muted, is to have a function that returns a static string:
the new osmo_sock_get_name2().
- (I think the osmo_sock_get_{local,remote}_* convenience wrappers should never
have been added, because they encourage the caller to invoke the same code
twice, for IP addr and port, and throw away one half each time.)
Related: Iae728192f499330d16836d9435648f6b8ed213b6 (osmo-mgw)
Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398
---
M include/osmocom/core/socket.h
M src/socket.c
2 files changed, 50 insertions(+), 17 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/68/12268/3
--
To view, visit https://gerrit.osmocom.org/12268
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398
Gerrit-Change-Number: 12268
Gerrit-PatchSet: 3
Gerrit-Owner: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Pau Espin Pedrol
Gerrit-CC: Stefan Sperling
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 2 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Tue, 18 Dec 2018 17:43:26 + Gerrit-HasComments: No Gerrit-HasLabels: Yes
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Stefan Sperling has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/12268/2/src/socket.c File src/socket.c: https://gerrit.osmocom.org/#/c/12268/2/src/socket.c@825 PS2, Line 825: /* "r=1.2.3.4:123<->l=5.6.7.8:987" */ Could we keep this comment in osmo_sock_get_name() also? Without that command the 1 + 5 + 3 + 2 is impossible to understand. -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 2 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-CC: Stefan Sperling Gerrit-Comment-Date: Tue, 18 Dec 2018 17:41:28 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 1: (2 comments) https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h File include/osmocom/core/socket.h: https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@64 PS1, Line 64: int osmo_sock_get_name2_buf(char *str, size_t str_len, int fd); > ok. Just to make sure, it's not confusing that osmo_sock_get_name_buf() > results in "r:1.2.3. […] The function name contains "buf" some some reason, so readers should be able to figure out there's 2 functions for some reason and that they are not exactly the same. Furthermore, future readers, as their name indicate, are capable of reading the function documentation and few lines of code :) So yes, let's please remove unneeded 2. https://gerrit.osmocom.org/#/c/12268/1/src/socket.c File src/socket.c: https://gerrit.osmocom.org/#/c/12268/1/src/socket.c@796 PS1, Line 796: * \param[out] str Destination string buffer. > lol what, a variable containing a space? I do this just for readability, I > find it a lot easier to s […] Not really, but it's not really important, you can leave it like that if you want. Just wanted to make sure at least you were aware of it. -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 1 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Tue, 18 Dec 2018 11:15:17 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Hello Pau Espin Pedrol, Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/12268
to look at the new patch set (#2).
Change subject: add to osmo_sock_get_name*() API
..
add to osmo_sock_get_name*() API
Basically, I am applying code review that I would have given had I not been on
vacation when the last osmo_sock_get_name* stuff was merged.
osmo_sock_get_name2() is so far a static internal function. However, it is
nothing like osmo_sock_get_name(), so instead rename it to
osmo_sock_get_ip_and_port(). Also make it public API, no need to hide it. I'm
adding an "and" in the name to hopefully clarify: "ip_port" vs. "ip_and_port"
-- there already are _get_X_ip_port() functions that only return the port
string, despite "ip" in the name.
Add new public osmo_sock_get_name2(), which is like osmo_sock_get_name(),
except it uses a static string instead of talloc, and omits the braces. This
is most convenient for log statement formats, avoiding dyn allocations.
Add new osmo_sock_get_name2_buf(), which is like osmo_sock_get_name2() but
writes to a caller provided char buffer. Technically this should be called
osmo_sock_get_name_buf() without '2', but that would be quite confusing.
Use osmo_sock_get_name2_buf() in the implementation of osmo_sock_get_name(),
but use another (non-static) local string buffer, because adding braces is too
complex without talloc_snprintf().
Rationale:
I want to improve the logging of socket errors, e.g. change
DLMGCP ERROR Failed to read: 111/Connection refused (mgcp_client.c:720)
to
DLMGCP ERROR Failed to read: r=10.0.99.2:2427<->l=10.0.99.2:2728:
111='Connection refused' (mgcp_client.c:721)
but it is just not handy to compose logging with the current API:
- osmo_sock_get_name() requires a talloc_free().
- all the others require output buffers.
- the only way to conveniently compose a logging string and,
- notably, the only trivial way to skip the string composition if the logging
level is currently muted, is to have a function that returns a static string:
the new osmo_sock_get_name2().
- (I think the osmo_sock_get_{local,remote}_* convenience wrappers should never
have been added, because they encourage the caller to invoke the same code
twice, for IP addr and port, and throw away one half each time.)
Related: Iae728192f499330d16836d9435648f6b8ed213b6 (osmo-mgw)
Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398
---
M include/osmocom/core/socket.h
M src/socket.c
2 files changed, 49 insertions(+), 17 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/68/12268/2
--
To view, visit https://gerrit.osmocom.org/12268
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398
Gerrit-Change-Number: 12268
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Neels Hofmeyr
Gerrit-Reviewer: Pau Espin Pedrol
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 2: (2 comments) https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h File include/osmocom/core/socket.h: https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@64 PS1, Line 64: int osmo_sock_get_name_buf(char *str, size_t str_len, int fd); > I mean it should be named osmo_sock_get_name_buf(). […] ok. Just to make sure, it's not confusing that osmo_sock_get_name_buf() results in "r:1.2.3.4:5<->l:4.5.6.7:8" while osmo_sock_get_name() returns a tallocd "(r:7:8)" with braces? (I will know this, but am worried about future readers) If you're sure then I'll drop the 2. https://gerrit.osmocom.org/#/c/12268/1/src/socket.c File src/socket.c: https://gerrit.osmocom.org/#/c/12268/1/src/socket.c@796 PS1, Line 796: * \param[out] str Destination string buffer. > Well since a variable in C cannot contain a space, it makes no sense to me to > add an extra space, it […] lol what, a variable containing a space? I do this just for readability, I find it a lot easier to spot where the doc string starts when setting it apart from the variable name. I'd even use a tab if I liked tabs. Would a tab be better? -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 2 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Tue, 18 Dec 2018 02:49:17 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 1: (2 comments) https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h File include/osmocom/core/socket.h: https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@64 PS1, Line 64: int osmo_sock_get_name2_buf(char *str, size_t str_len, int fd); > there is a 2? You mean it should be in the end instead? […] I mean it should be named osmo_sock_get_name_buf(). Let's not extend use of "2" because a similar "epic FAIL" API has it. https://gerrit.osmocom.org/#/c/12268/1/src/socket.c File src/socket.c: https://gerrit.osmocom.org/#/c/12268/1/src/socket.c@796 PS1, Line 796: * \param[out] str Destination string buffer. > you mean the indent? I'm just copying the scheme I find around here. […] Well since a variable in C cannot contain a space, it makes no sense to me to add an extra space, it's obvious the var ends at whitespace boundary, so no need to add 2 of them. -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 1 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Mon, 17 Dec 2018 14:39:33 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 1: (3 comments) https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h File include/osmocom/core/socket.h: https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@63 PS1, Line 63: const char *osmo_sock_get_name2(int fd); > what about osmo_sock_get_name_static()? we have many other _get_name() functions that return strings in a static buffer (also sometimes in a buffer that belongs to the struct or is part of a value_string[]). This was the exception, so to me it feels right to imply 'static'. https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@64 PS1, Line 64: int osmo_sock_get_name2_buf(char *str, size_t str_len, int fd); > no need to have a 2 here. there is a 2? You mean it should be in the end instead? The right way would be to call it osmo_sock_get_name_buf() since it's the first signature with this name, but that would confuse with the semantics of osmo_sock_get_name(). I think osmo_sock_get_name2_buf() shows that it intimately belongs to osmo_sock_get_name2() and that we don't need to go looking for a first API version of it. But feel free to request a different name. https://gerrit.osmocom.org/#/c/12268/1/src/socket.c File src/socket.c: https://gerrit.osmocom.org/#/c/12268/1/src/socket.c@796 PS1, Line 796: * \param[out] str Destination string buffer. > 2 whitespace here and in next line. you mean the indent? I'm just copying the scheme I find around here. I think it's rather weird to indent twice for \param, but that's what this file does, probably because most just have a summary doc and line up with '/*!'. Or do you mean the 2xspace in '\param foo The foo'? I always do that to set apart the argument name from its actual explanation, since doxygen format lacks a ':' that I would like to put there instead. -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 1 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Neels Hofmeyr Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Mon, 17 Dec 2018 13:50:10 + Gerrit-HasComments: Yes Gerrit-HasLabels: No
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/12268 ) Change subject: add to osmo_sock_get_name*() API .. Patch Set 1: Code-Review+1 (3 comments) https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h File include/osmocom/core/socket.h: https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@63 PS1, Line 63: const char *osmo_sock_get_name2(int fd); what about osmo_sock_get_name_static()? https://gerrit.osmocom.org/#/c/12268/1/include/osmocom/core/socket.h@64 PS1, Line 64: int osmo_sock_get_name2_buf(char *str, size_t str_len, int fd); no need to have a 2 here. https://gerrit.osmocom.org/#/c/12268/1/src/socket.c File src/socket.c: https://gerrit.osmocom.org/#/c/12268/1/src/socket.c@796 PS1, Line 796: * \param[out] str Destination string buffer. 2 whitespace here and in next line. -- To view, visit https://gerrit.osmocom.org/12268 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398 Gerrit-Change-Number: 12268 Gerrit-PatchSet: 1 Gerrit-Owner: Neels Hofmeyr Gerrit-Reviewer: Jenkins Builder (102) Gerrit-Reviewer: Pau Espin Pedrol Gerrit-Comment-Date: Wed, 12 Dec 2018 12:49:09 + Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
Change in libosmocore[master]: add to osmo_sock_get_name*() API
Neels Hofmeyr has uploaded this change for review. (
https://gerrit.osmocom.org/12268
Change subject: add to osmo_sock_get_name*() API
..
add to osmo_sock_get_name*() API
Basically, I am applying code review that I would have given had I not been on
vacation when the last osmo_sock_get_name* stuff was merged.
osmo_sock_get_name2() is so far a static internal function. However, it is
nothing like osmo_sock_get_name(), so instead rename it to
osmo_sock_get_ip_and_port(). Also make it public API, no need to hide it. I'm
adding an "and" in the name to hopefully clarify: "ip_port" vs. "ip_and_port"
-- there already are _get_X_ip_port() functions that only return the port
string, despite "ip" in the name.
Add new public osmo_sock_get_name2(), which is like osmo_sock_get_name(),
except it uses a static string instead of talloc, and omits the braces. This
is most convenient for log statement formats, avoiding dyn allocations.
Add new osmo_sock_get_name2_buf(), which is like osmo_sock_get_name2() but
writes to a caller provided char buffer. Technically this should be called
osmo_sock_get_name_buf() without '2', but that would be quite confusing.
Use osmo_sock_get_name2_buf() in the implementation of osmo_sock_get_name(),
but use another (non-static) local string buffer, because adding braces is too
complex without talloc_snprintf().
Rationale:
I want to improve the logging of socket errors, e.g. change
DLMGCP ERROR Failed to read: 111/Connection refused (mgcp_client.c:720)
to
DLMGCP ERROR Failed to read: r=10.0.99.2:2427<->l=10.0.99.2:2728:
111='Connection refused' (mgcp_client.c:721)
but it is just not handy to compose logging with the current API:
- osmo_sock_get_name() requires a talloc_free().
- all the others require output buffers.
- the only way to conveniently compose a logging string and,
- notably, the only trivial way to skip the string composition if the logging
level is currently muted, is to have a function that returns a static string:
the new osmo_sock_get_name2().
- (I think the osmo_sock_get_{local,remote}_* convenience wrappers should never
have been added, because they encourage the caller to invoke the same code
twice, for IP addr and port, and throw away one half each time.)
Related: Iae728192f499330d16836d9435648f6b8ed213b6 (osmo-mgw)
Change-Id: I8ad89ac447c9c582742e70d082072bdd40a5a398
---
M include/osmocom/core/socket.h
M src/socket.c
2 files changed, 49 insertions(+), 17 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/68/12268/1
diff --git a/include/osmocom/core/socket.h b/include/osmocom/core/socket.h
index 79a61bb..fd28f21 100644
--- a/include/osmocom/core/socket.h
+++ b/include/osmocom/core/socket.h
@@ -60,6 +60,9 @@
const char *socket_path, unsigned int flags);
char *osmo_sock_get_name(void *ctx, int fd);
+const char *osmo_sock_get_name2(int fd);
+int osmo_sock_get_name2_buf(char *str, size_t str_len, int fd);
+int osmo_sock_get_ip_and_port(int fd, char *ip, size_t ip_len, char *port,
size_t port_len, bool local);
int osmo_sock_get_local_ip(int fd, char *host, size_t len);
int osmo_sock_get_local_ip_port(int fd, char *port, size_t len);
int osmo_sock_get_remote_ip(int fd, char *host, size_t len);
diff --git a/src/socket.c b/src/socket.c
index e804ab5..25c5504 100644
--- a/src/socket.c
+++ b/src/socket.c
@@ -697,10 +697,7 @@
return osmo_fd_init_ofd(ofd, osmo_sock_unix_init(type, proto,
socket_path, flags));
}
-/*! Get the IP and/or port number on socket. This is for internal usage.
- * Convenience wrappers: osmo_sock_get_local_ip(),
- * osmo_sock_get_local_ip_port(), osmo_sock_get_remote_ip(),
- * osmo_sock_get_remote_ip_port() and osmo_sock_get_name()
+/*! Get the IP and/or port number on socket in separate string buffers.
* \param[in] fd file descriptor of socket
* \param[out] ip IP address (will be filled in when not NULL)
* \param[in] ip_len length of the ip buffer
@@ -709,7 +706,7 @@
* \param[in] local (true) or remote (false) name will get looked at
* \returns 0 on success; negative otherwise
*/
-static int osmo_sock_get_name2(int fd, char *ip, size_t ip_len, char *port,
size_t port_len, bool local)
+int osmo_sock_get_ip_and_port(int fd, char *ip, size_t ip_len, char *port,
size_t port_len, bool local)
{
struct sockaddr sa;
socklen_t len = sizeof(sa);
@@ -741,7 +738,7 @@
*/
int osmo_sock_get_local_ip(int fd, char *ip, size_t len)
{
- return osmo_sock_get_name2(fd, ip, len, NULL, 0, true);
+ return osmo_sock_get_ip_and_port(fd, ip, len, NULL, 0, true);
}
/*! Get local port on socket
@@ -752,7 +749,7 @@
*/
int osmo_sock_get_local_ip_port(int fd, char *port, size_t len)
{
- return osmo_sock_get_name2(fd, NULL, 0, port, len, true);
+ return osmo_sock_get_ip_and_port(fd, NULL, 0, port, len, true);
}
/*! Get remote IP address on
