Change in libosmocore[master]: add osmo_ip_port API

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Tue Mar 5 22:50:41 UTC 2019


Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13123 )

Change subject: add osmo_ip_port API
......................................................................


Patch Set 4:

(2 comments)

https://gerrit.osmocom.org/#/c/13123/4/include/osmocom/core/ip_port.h
File include/osmocom/core/ip_port.h:

https://gerrit.osmocom.org/#/c/13123/4/include/osmocom/core/ip_port.h@47
PS4, Line 47: osmo_ip_port
> basically this looks like you're reinventing sockaddr_storage which is usually typecast to sockaddr_ […]
- sockaddr stores the port in network byte order, each log line would have to remember to call ntohs().
- we want the IP address string for
  - MGCP messages
  - straightforward logging
- in MGCP the string comes in from MGCP messages, and I want to convert that to in_addr or sockaddr_in in a standardized way.

In the end, a string is a string and a uint16_t is a uint16_t without any special cases about them,
and the sockaddr API is just a lot of clutter around that.

(at first, this implementation had no 'af' indicating AF_INET or AF_INET6, maybe it would be better to drop that and leave it to string interpretation, see osmo_ip_str_type())

I could also actually place this in osmo-mgw.git, as its usefulness is associated with MGCP, and then we'd avoid the ARM architecture nonsense where netinet/in.h isn't known...?


https://gerrit.osmocom.org/#/c/13123/4/include/osmocom/core/ip_port.h@51
PS4, Line 51: 46
> INET6_ADDRSTRLEN in arpa/inet. […]
I had used that at first, but it causes a dependency on #include <netinet/in.h> which then fails to compile on ARM. 

Then it would be nice to conditionally include that header, but we must not include "config.h" in system-installed header files.

That is the single reason why I am using a magic number.
If you look in the .c file then you'll see there is an osmo_static_assert ensuring the size >= INET6_ADDRSTRLEN

Oh wait, arpa/inet.h you say?



-- 
To view, visit https://gerrit.osmocom.org/13123
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id617265337f09dfb6ddfe111ef5e578cd3dc9f63
Gerrit-Change-Number: 13123
Gerrit-PatchSet: 4
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-Comment-Date: Tue, 05 Mar 2019 22:50:41 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190305/4a71bc1f/attachment.html>


More information about the gerrit-log mailing list