pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/16202 )
Change subject: add libosmo-mslookup and mDNS implementation ...................................................................... Patch Set 9: (25 comments) I really got fed up of reviewing this commit, it's a monster. There's tons of new APIs, objects, and different layers. I could not find a good explanation on the relations so it's almost impossible not getting lost. IIUC there's several layers of objects using lower layers. Please provide separate commits with an introduction for each layer and how it will be used by other objects and which objects will it use. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/configure.ac File configure.ac: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/configure.ac@182 PS9, Line 182: libosmo-mslookup.pc Does it make sense to make it a separate shared library if it's only used by osmo-hlr? are we sure it will be used by someone else (osmo-msc)? https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/debian/control File debian/control: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/debian/control@75 PS9, Line 75: Depends: ${shlibs:Depends}, ${shlibs:Depends} can be dropped I think. Maybe it needs to be ${misc:Depends} from looking at line 52. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/hlr/logging.h File include/osmocom/hlr/logging.h: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/hlr/logging.h@11 PS9, Line 11: DMSLOOKUP, Guinness record of longest log category name. Please shrink it. 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@1 PS9, Line 1: /* Copyright 2019 by sysmocom s.f.m.c. GmbH <[email protected]> Looks like this file and mdns.h can be merged. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mdns_sock.h@26 PS9, Line 26: struct addrinfo *ai; header missing for struct addrinfo. 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), which kind of callback is this one? osmo_fd_cb? 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); const mdns_sock? strange since I'd expect send() returning error means socket must be closed and set to -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@29 PS9, Line 29: #define OSMO_MSLOOKUP_SERVICE_HLR_GSUP "gsup.hlr" We may want a str_value here instead 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); iiuc that's initializing the struct and not finding it. In that case osmo_mslookup_query_init_from_domain_str seems more explicit. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h File include/osmocom/mslookup/mslookup_client.h: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h@35 PS9, Line 35: /*! This part of a lookup request is not seen by the individual query method implementations. */ Is this internal stuff then? Are we sure we are not installing it with autotools then? https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/include/osmocom/mslookup/mslookup_client.h@95 PS9, Line 95: struct osmo_mslookup_client_method { There's no allocator/constructor for struct osmo_mslookup_client_method ? Looks like each implementation has its own constructor? Please document so here. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/libosmo-mslookup.pc.in File libosmo-mslookup.pc.in: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/libosmo-mslookup.pc.in@9 PS9, Line 9: Libs: -L${libdir} @TALLOC_LIBS@ -losmogsm -losmo-mslookup -losmocore is -losmogsm really needed? 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@76 PS9, Line 76: msgb_free(msg); missing "talloc_free(req.domain);" here? Better use goto for ordered cleanup. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@86 PS9, Line 86: * NULL on error. No need for new line. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@106 PS9, Line 106: if (req) this if is not needed. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns.c@108 PS9, Line 108: if (query) this if is not needed. 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); use osmo_loadbe or whatever here, you may incurr into unaligned access? 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@92 PS9, Line 92: * -EINVAL on error. new line not needed. 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) this looks like function should be called encode_append or alike. 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); are you sure "len" used here is correct? https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c@39 PS9, Line 39: ret->length = len + 1; and here. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_record.c@45 PS9, Line 45: * NULL on error. all around: new line not needed. https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_rfc.c File src/mslookup/mdns_rfc.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_rfc.c@122 PS9, Line 122: osmo_store16be(buf->id, &buf->id); wtf? is it really worth memcpying everything then changing tons of stuff in place? All this is a bit strange imho, you are using packed structs with host-endian values... https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_sock.c File src/mslookup/mdns_sock.c: https://gerrit.osmocom.org/c/osmo-hlr/+/16202/9/src/mslookup/mdns_sock.c@40 PS9, Line 40: * will not only be called when someone else is sending data, but also for data that was sent from this osmo_mdns_sock. "was sent" or "is to be sent" / "can be sent"? 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]) { what are you actually checking here? if First byte/octet in address is not zero? why? -- 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: 9 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith <[email protected]> Gerrit-CC: pespin <[email protected]> Gerrit-Comment-Date: Mon, 25 Nov 2019 18:32:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
