Attention is currently required from: laforge, pespin, fixeria, msuraev.
12 comments:
Patchset:
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:
Patch Set #13, Line 244: OSMO_STRBUF_PRINTF(*sb, "%s%s", oss->ip, after);
We can't use osmo_sockaddr_to_str_buf2 because it always prints port in addition to host. […]
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:
Patch Set #16, Line 205: osmo_strbuf
there no longer is an osmo_strbuf arg (which i like =)
Patch Set #16, Line 216: size_t count = use_hosts ? host_count : addrs_count;
what? why do you reuse the same function with 2 sets of different parameters? this is utterly confus […]
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
Patch Set #16, Line 218: after
("suffix" is a better name and a word that doesn't also mean "butt")
Patch Set #16, Line 240: osmo_sockaddr_str
no osmo_sockaddr_str plz
just use &osa below
Patch Set #16, 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 =)
Patch Set #16, Line 258: OSMO_STRBUF_PRINTF(sb, ")");
don't you also need a closing brace if count > 1?
Patch Set #16, 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)
Patch Set #16, Line 263: static void multiaddr_log_helper(uint8_t loglevel, const char *fmt, uint16_t port, const char **hosts, size_t host_cnt)
I see no point in having this helper, with a formatting string which then anyway implies a given ord […]
yes, didn't i actually post a similar comment before?
Patch Set #16, 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 change 28993. To unsubscribe, or for help writing mail filters, visit settings.