neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )
Change subject: add libosmo-mslookup and mDNS implementation ...................................................................... Patch Set 11: (7 comments) https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h File include/osmocom/mslookup/mdns_sock.h: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@30 PS9, Line 30: int (*cb)(struct osmo_fd *fd, unsigned int what), > > the API doc in mdns_sock.c hints at it, but I'm adding a comment there. […] (sorry for confusion, I meant a gerrit review comment) https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@32 PS9, Line 32: int osmo_mdns_sock_send(const struct osmo_mdns_sock *mdns_sock, struct msgb *msg); > Looking at sendto being used in libosmocore, we don't implicitly close the > socket. […] does feel slightly weird to get a const of something used to send a message, but it is in fact appropriate here. +1 https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h File include/osmocom/mslookup/mslookup.h: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup.h@113 PS9, Line 113: int osmo_mslookup_query_from_domain_str(struct osmo_mslookup_query *q, const char *domain); > Done (Neels renamed it, just replying so I can mark this as "resolved" in > gerrit. […] (btw, I never bother with those "Resolved" tick marks in gerrit at all... they sometimes get set automatically, apparently, but I feel like we don't need to pay attention to them) https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c File src/mslookup/mdns.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@127 PS9, Line 127: osmo_sockaddr_str_from_32(sockaddr_str, *(uint32_t *)rec->data, 0); > osmo_sockaddr_str_from_32 expects the input to be network-byte-order. […] pespin's point here is that the beginning of the uint32_t may be at a memory address that doesn't match a word aligned position. So the CPU expects a uint32_t to start at an address that is a multiple of 4, which is why we need to first copy the four bytes to a local variable before feeding to osmo_sockaddr_str_from_32(). Might even qualify for an osmo_sockaddr_str_from_32p() as in a pointer which takes care of aligning? I guess _from_32() should have taken a pointer as argument from the start :/ For a moment there I thought passing as a uint32_t argument to a func should imply copying; checking: #include <stdio.h> #include <stdint.h> uint8_t bytes[8] = { 1, 2, 3, 4, 5, 6, 7, 8 }; void uint32_func(uint32_t val) { printf("%04x\n", val); } int main() { int i; for (i = 0; i < 4; i++) uint32_func(*(uint32_t*)(&bytes[i])); return 0; } gcc -o x -fsanitize=address -fsanitize=undefined x.c ./x 4030201 x.c:15:17: runtime error: load of misaligned address 0x5609c31d00a1 for type 'uint32_t', which requires 4 byte alignment 0x5609c31d00a1: note: pointer points here 00 00 00 01 02 03 04 05 06 07 08 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ^ 5040302 6050403 7060504 So we need to align. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_msg.c File src/mslookup/mdns_msg.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_msg.c@114 PS9, Line 114: if (osmo_mdns_rfc_record_encode(ctx, msg, &rec) != 0) > I'm a bit undecided about this, since all osmo_mdns_rfc_*_encode functions > with a msgb argument are […] Let's keep the name "_encode", not add "_append". For exactly those examples that you show; no other function encoding to the end of a msgb has "append" in its name. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c File src/mslookup/mdns_record.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c@35 PS9, Line 35: ret->data = (uint8_t *)talloc_asprintf(ctx, "%c%s=%s", (char)len, key, value); > Yes, I'm sure that this is correct. […] maybe they were writing the DNS PoC in Turbo Pascal or something. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mslookup.c File src/mslookup/mslookup.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mslookup.c@148 PS9, Line 148: if (result->host_v4.ip[0]) { > NULL character You mean the nul character :) We won't get around pointer or array arithmetic, after all it is a pointer to a char array. I've gotten "what's this?" review before for if (*str) and that's why I used if (str[0]) Would you prefer an osmo_str_is_empty(str) macro? IMHO we don't need that and C string logic is common enough that we can do either *str or str[0] or *str != '\0' by author's preference. IMHO this code is obviously printing a v4 addr when it is set, I'm leaving it like it is... -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/16202 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I83487ab8aad1611eb02e997dafbcb8344da13df1 Gerrit-Change-Number: 16202 Gerrit-PatchSet: 11 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Comment-Date: Wed, 27 Nov 2019 02:17:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: osmith <[email protected]> Gerrit-MessageType: comment
