This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
neels gerrit-no-reply at lists.osmocom.orgneels 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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 27 Nov 2019 02:17:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Comment-In-Reply-To: osmith <osmith at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191127/3f317afd/attachment.htm>