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) 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 &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-CC: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Comment-Date: Thu, 30 Nov 2023 23:39:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: comment