Attention is currently required from: msuraev. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/28992 ) Change subject: Add osmo_sockaddr_strs_to_str() ...................................................................... Patch Set 8: (9 comments) Patchset: PS8: I'm sorry that I'm late for this patch review. This patch has a number of rough edges, I would welcome if we could revert it and get some more review cycles. File src/sockaddr_str.c: https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/6219adc6_145bfcb8 PS8, Line 547: * buf_len >= 2: (hostA|hostB|...|...) buf_len is the buffer size of the output string buffer *buf. A buf_len == 0 cannot return any "()" string, it has zero size. buf_len == 1 cannot contain "hostA". Did you mean sa_str_count? https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/d27293a9_acbb2b9e PS8, Line 555: int osmo_sockaddr_strs_to_str(char *buf, size_t buf_len, const struct osmo_sockaddr_str **sa_str, size_t sa_str_count) i find it odd that you want to pass an array of osmo_sockaddr_str POINTERS, instead of an array of osmo_sockaddr_str instances. i.e. i would expect an arg like 'const struct osmo_sockaddr_str *sa_strs', not '**'. https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/316d840f_674d4fd8 PS8, Line 562: return -ENOMEM; this check should not be here. The idea is to always return the nr of characters needed to output the full string, regardless of the underlying buffer length. If you return -ENOMEM on some specific buffer lengths, you destroy the ability of finding a sufficient buffer length from the return value. The OSMO_STRBUF macros deal with all cases of insufficient buffer length, just let it play through to the end, so that the proper sb.chars_needed is returned. https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/fdcd0fdd_b3bde8dc PS8, Line 567: for (i = 0; i < sa_str_count; i++) { for (...) { if (i) OSMO_STRBUF_PRINTF(sb, "|"); if (sa_str[i]) OSMO_STRBUF_PRINTF(sb, OSMO_SOCKADDR_STR_NO_PORT_FMT, OSMO_SOCKADDR_STR_NO_PORT_FMT_ARGS(sa_str[i])); } if (sa_str_count != 1) OSMO_STRBUF_PRINTF(sb, ")"); File tests/sockaddr_str/sockaddr_str_test.c: https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/078d4d8b_e4da2349 PS8, Line 28: #include <osmocom/core/logging.h> why add application and logging? there should be no select loop. nothing about osmo_sockaddr_str should ever cause osmocom logging. https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/a57e7473_891bec86 PS8, Line 268: struct osmo_sockaddr_str *sa_strs[OSMO_SOCK_MAX_ADDRS]; (...and here just a struct osmo_sockaddr_str sa_strs[N], no need to talloc instances below) https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/998f5771_bdf3e063 PS8, Line 271: for (i = 0, j = 0; j < count && i < OSMO_SOCK_MAX_ADDRS; j++) {//ARRAY_SIZE(oip_data) can we drop the // comments? https://gerrit.osmocom.org/c/libosmocore/+/28992/comment/146c54fa_9c1e2c65 PS8, Line 290: }; why add logging infrastructure?? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/28992 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ic0d7c08f669994e37a2314555ecac85d28c42c89 Gerrit-Change-Number: 28992 Gerrit-PatchSet: 8 Gerrit-Owner: msuraev &lt;msuraev(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: msuraev &lt;msuraev(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-CC: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: msuraev &lt;msuraev(a)sysmocom.de&gt; Gerrit-Comment-Date: Wed, 17 Aug 2022 00:34:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment