Attention is currently required from: laforge, pespin, fixeria, msuraev. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/28993 ) Change subject: Update multiaddr helper ...................................................................... Patch Set 16: (12 comments) Patchset: PS16: In this patch and its friends there is so much complexity for a relatively trivial thing to do, and this review has been going on for such a long time, and so much review ping pong has been going on. Sorry to say it but slowly but surely i feel like i'd rather be implementing this for you instead of reviewing it any longer... File src/core/socket.c: https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/ac7bdf9f_c9c46925 PS13, Line 244: OSMO_STRBUF_PRINTF(*sb, "%s%s", oss->ip, after); ok that's true, I missed that. but you could do struct osmo_sockaddr osa = { .u.sa = add }; (struct osmo_sockaddr is much more efficient on memory and processing than osmo_sockaddr_str -- the osmo_sockaddr_str is about converting string to sockaddr, not such a good fit for the other way) File src/core/socket.c: https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/d22d48d5_d25cde57 PS16, Line 205: osmo_strbuf there no longer is an osmo_strbuf arg (which i like =) https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/d609ba31_98366eb0 PS16, Line 216: size_t count = use_hosts ? host_count : addrs_count; funny how you add a bool use_hosts while the var hosts itself already can serve as a bool =) oh wait, how about use_hosts = hosts ? (host_count > 0) : false or actually also what Pau said, but not so critical for a static function https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/d0e24b08_84ecc282 PS16, Line 218: after ("suffix" is a better name and a word that doesn't also mean "butt") https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/209057bf_81f97d53 PS16, Line 240: osmo_sockaddr_str no osmo_sockaddr_str plz https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/8c27864a_a03a177c PS16, Line 242: osa just use &osa below https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/9c2e110d_f5ac4042 PS16, Line 248: rc = osmo_sockaddr_str_to_sockaddr(&oss, &osap->u.sas); convert a sockaddr to string and then back??? nooooooo! osa.u.sa = addrs[i]; and you're done =) https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/ce750692_bdcfb3ed PS16, Line 258: OSMO_STRBUF_PRINTF(sb, ")"); don't you also need a closing brace if count > 1? https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/6af95c22_295229dc PS16, Line 263: helper (I am against the word "helper" in function names, all functions are helpers; instead the name should convey what the function actually does) https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/94baf516_d968cb36 PS16, Line 263: static void multiaddr_log_helper(uint8_t loglevel, const char *fmt, uint16_t port, const char **hosts, size_t host_cnt) yes, didn't i actually post a similar comment before? https://gerrit.osmocom.org/c/libosmocore/+/28993/comment/f09aa30d_6a39d373 PS16, Line 267: multiaddr_snprintf(strbuf, 512, hosts, host_cnt, NULL, 0); this snprintf + LOG should be surrounded by an "if (log_check_level)" condition to skip it if this logging is disabled everywhere -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/28993 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Icef53fe4b6e51563d97a1bc48001d67679b3b6e9 Gerrit-Change-Number: 28993 Gerrit-PatchSet: 16 Gerrit-Owner: msuraev <msuraev(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: msuraev <msuraev(a)sysmocom.de> Gerrit-Comment-Date: Mon, 06 Mar 2023 02:06:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Comment-In-Reply-To: msuraev <msuraev(a)sysmocom.de> Gerrit-MessageType: comment