net: sntp: Add "simple" one-shot API to query SNTP server #15602

pfalcon · 2019-04-22T15:10:22Z

Many real-world tasks require absolute time on devices which otherwise lack RTC. A realistic way then to use SNTP to get the current time. There should be a way to do that quickly and easily. Introduced in this PR. (This PR also includes patches from #15582 & #15600 as dependencies)

pfalcon · 2019-04-22T15:11:23Z

@d3zd3z, @galak: GoogleIoT SNTP support I'm working on depends on this patch.

jukkar · 2019-04-24T08:12:19Z

include/net/netutils.h

We have already net_ip.h which contains various helper functions, so there is no need to create a new header file for just these.

We have already net_ip.h which contains various helper functions, so there is no need to create a new header file for just these.

So helper functions are for Zephyr network stack. These are application-level helpers on top of POSIX API.

Why not call this utils.h as the net is redundant info here as the file is in net directory anyway. We have other net_ prefixed header files in the net directory but it feels unnecessary now, unfortunately we cannot really change that at this point.

Why not call this utils.h as the net is redundant info here as the file is in net directory anyway.

As I mentioned, the idea was to make it have more selective name than just often-overused "utils", in particular, because of the idea to use it e.g. in POSIX (Linux) app builds, where "net/" prefix might be there or not.

Calling it just net/utils.h might work, but based on the previous discussion, it seems it's worth emphasizing that it's socket API level utils, so let me try socketutils.h and see if that works. (Also considered sockutils.h, but that may be confused with utils for SOCKS protocol for example).

subsys/net/lib/utils/CMakeLists.txt

subsys/net/lib/utils/addr_utils.c

pfalcon · 2019-04-25T11:02:55Z

@rlubos: This PR is what I was talking about previously re: API design. Once we have a set of good, generic, composable API primitives (*), we can provide whatever convenience APIs on top of them as needed (or even wished). For example, here the usecase and requirement is "Suppose you have a POSIX app, which of course expect a proper absolute time to be available, and you want to port it to Zephyr. It should be possible to obtain the absolute time with ~just one line (function call) (including no extra legwork on preparing params to this function call)".

SNTP API is actually not such, and needs further work, e.g. currently it's synchronously only, which is not adequate.

vanti · 2019-04-26T16:44:47Z

include/net/sntp.h

It'd be good to document the return value of this function.

It'd be good to document the return value of this function.

Fixed, thanks.

vanti · 2019-04-26T16:46:45Z

include/net/sntp.h

I think 'sntp_request()' should be 'sntp_query()'

I think 'sntp_request()' should be 'sntp_query()'

Good catch, fixed.

jukkar · 2019-05-02T10:50:35Z

subsys/net/lib/utils/addr_utils.c

NI_MAXHOST would be better here, we just need to define it somewhere.

NI_MAXHOST would be better here

Note that NI_MAXHOST is not in POSIX:
http://pubs.opengroup.org/cgi/susv4search.cgi?KEYWORDS=NI_MAXHOST&SUBSTRING=substring&CONTEXT=

Let me hack it up, but we'd better get the basics right, then add extensions properly.

Note that NI_MAXHOST is not in POSIX:

I am not claiming it is in Posix. I am just saying that having magic value here is bad, thus I proposed this symbol.

jukkar · 2019-05-02T10:52:59Z

subsys/net/lib/utils/addr_utils.c

If we are calling this from user app, then we need to have more null checks for the input parameters.

If we are calling this from user app, then we need to have more null checks for the input parameters.

Well, why? What's so special about NULL? What we'd be interested in catching any invalid value, but we can't, because user can write char *foo and get uninitiated var with value from stack. In this regard, NULL is just one of billions invalid value. If user passes NULL, the app will crush with a nice stacktrace (if built with suitable options), and we check for NULL, it doesn't crash there, but crashes elsewhere, or just silently misbehaves, in which regard checking for NULL (out of all other possible invalid values) is more trouble than boon.

Also, isn't checking argument one of those things the "Zephyr way" goes against? From a security perspective, this isn't really the best place to be checking values, and the ones that matter are those that come from external to the system.

As this function is to be callable from user space, it makes sense to validate the input.

As this function is to be callable from user space, it makes sense to validate the input.

It makes sense, but we can't really do that, as explained above, just can pretend to do that. All those NULL checks just add code size, but surprisingly, efficiency still seems to be a concern for Zephyr: #15969.

So, I'll add it here, but we'll end up needing to add CONFIG_DISABLE_NULL_CHECKS to disable all those useless checks sprinkled around and get the code size back.

As this function is to be callable from user space

And this argument... These functions are themselves userspace, they are just application convenience functions, factored out for reuse. It's not like that they're kernel-level functions, params to which should be validated (much more thoroughly than NULL checks).

Anyway, added NULL check.

jukkar · 2019-05-02T10:55:03Z

subsys/net/lib/utils/addr_utils.c

Better host[host_len] = '\0'; as typically we write the terminating null like that.

Better host[host_len] = '\0'; as typically we write the terminating null like that.

It's so 1980ies, but if we typically write it like that, fixed.

jukkar · 2019-05-02T10:57:22Z

subsys/net/lib/utils/addr_utils.c

As the functionality is almost identical to getaddrinfo(), why not call it net_getaddrinfo_str() as the addr looks redundant information.

As the functionality is almost identical to getaddrinfo(), why not call it net_getaddrinfo_str() as the addr looks redundant information.

The idea was to provide enough information and differentiation from raw getaddrinfo(), while keep it reasonably short. In that regard, as string in format host[:port] is called "address string", abbreviated addr_str, and that infix is used consistently in both function names, net_addr_str_find_port() and net_getaddrinfo_addr_str().

Even more selective name, like addr_str_w_port, would be better, but would lead to long function name. OTOH, just using "str" isn't selective enough IMHO.

jukkar · 2019-05-02T10:58:23Z

subsys/net/lib/utils/addr_utils.c

Why do we expose this function to user application, does it really need it? Could we just make it static?

Why do we expose this function to user application, does it really need it? Could we just make it static?

Yes, the idea was to expose both functions to user apps, to give them the flexibility. For example, if a user apps needs address only in numeric format, it can use net_addr_str_find_port() to parse it into addr vs port substrings, and then use inet_pton(), and avoid dependency on getaddrinfo().

I think we should keep this as static, it can be made global if really needed by someone. We should try to minimize the interfaces that we expose and this function is just too low level that application programmer would even find it in the API listing.

We should try to minimize the interfaces that we expose and this function is just too low level that application programmer would even find it in the API listing.

We should (finally) provide good and stable API set. And good API design exposes "atomic" functions, which can be combined by user to achieve any desired effect, while also providing higher-level convenience functions for commonly desired effect. That's exactly how this API is laid out. The API which offers only higher-level functionality and locks out user from using it, or require rewriting it completely to adjust some behavior, is well, bad API.

I don't see why all this concern about making that function static. Making it static is inviting someone to reinvent it in case of need. And the need can be easily anticipated.

I just want to avoid exposing functions if there is no real need for them. It is a pain to decprecate them if we find out we need to tweak the API or remove them.

I just want to avoid exposing functions if there is no real need for them.

While this PR is titled as SNTP-related, one of the subtasks was to start laying out helper API for apps (including POSIX apps) to reuse, instead of coding it again and again. So I did spend mental effort on factoring out that func to be (re)usable by other parts. That's why I'm concerned that this effort goes to /dev/null by request to static'ize it.

It is a pain to decprecate them if we find out we need to tweak the API or remove them.

So again, that func just does a useful thing on its own (parses address in format host[:host]), and intended for reuse, so I don't see it being under a reasonable risk of deprecation. (I'd be more concerned with visibly adhoc codes like #15636 - that doesn't implement general enough API, so would be subject of long incremental tweaks and partial deprecation, or will need to be replaced with a new more generic impl).

That said, if whole this discussion doesn't convince you, and you'd rather see "static" now, hopefully removed later, I'd rather move on with the matter.

This is not that big of an issue. I just have been burned before because of introducing some functions that were "useful" too, but at the end they were really not useful after all.

jukkar · 2019-05-02T11:12:57Z

include/net/netutils.h

Why not call this utils.h as the net is redundant info here as the file is in net directory anyway. We have other net_ prefixed header files in the net directory but it feels unnecessary now, unfortunately we cannot really change that at this point.

pfalcon · 2019-05-07T16:32:53Z

@PiotrZierhoffer, @tgorochowik: This is an example of paving the road for "wider compliant POSIX env" setup. For me, this is intermediate step, it should culminate in having a config option like CONFIG_GIMME_REAL_TIME, and then when application start up, gettimeofday() returning proper POSIX compliant timestamp.

pfalcon · 2019-05-07T19:38:30Z

@jukkar: Thanks for the review. I addressed some points, and explained why other points were done as they were, which hopefully makes sense.

tgorochowik · 2019-05-08T09:20:52Z

subsys/net/lib/sntp/sntp_simple.c

Shouldn't there be a define for the default port? Or at least a comment saying what it is?

123 is the standard (S)NTP port per the RFC. Added comment on that.

tgorochowik · 2019-05-08T09:21:10Z

include/net/socketutils.h

Redundant newline

Not in POSIX. Linux man getnameinfo says about it: "In order to assist the programmer in choosing reasonable sizes for the supplied buffers, <netdb.h> defines the constants #define NI_MAXHOST 1025 #define NI_MAXSERV 32 Since glibc 2.8, these definitions are exposed only if suitable feature test macros are defined, namely: _GNU_SOURCE, _DEFAULT_SOURCE (since glibc 2.19), or (in glibc versions up to and including 2.19) _BSD_SOURCE or _SVID_SOURCE." Signed-off-by: Paul Sokolovsky <[email protected]>

Two utils to manipulate addresses in format "addr[:port]". I.e., network address (domain name or numeric), optionally followed by port number: * net_addr_str_find_port(), to return pointer to port number substring (or NULL if not present). * net_getaddrinfo_addr_str(), which is effectively getaddrinfo() wrapper taking a "addr[:port]" string as a parameter. The header file is named socketutils.h to emphasize that these utility functions are implemented on top of BSD Sockets API (and other POSIX/ANSI C functions), and thus portable to other POSIX systems (e.g., Linux), so can be used in apps testing POSIX compatibility. More utility functions (beyond address manipulation) can be added later. Signed-off-by: Paul Sokolovsky <[email protected]>

sntp_simple() function queries the server (passed as "addr[:port]" string). It wraps calls to a number of other functions, and may be useful to write simple, concise apps needing the absolute time. Signed-off-by: Paul Sokolovsky <[email protected]>

pfalcon · 2019-05-08T10:14:58Z

Pushed new version addressing latest comments.

jukkar · 2019-05-09T12:18:13Z

subsys/net/lib/utils/addr_utils.c

This is not that big of an issue. I just have been burned before because of introducing some functions that were "useful" too, but at the end they were really not useful after all.

pfalcon added the area: Networking label Apr 22, 2019

pfalcon requested review from d3zd3z, galak and jukkar April 22, 2019 15:10

pfalcon mentioned this pull request Apr 23, 2019

net: sntp: Add sntp_query() function with fractional precision #15600

Merged

jukkar requested changes Apr 24, 2019

View reviewed changes

pfalcon force-pushed the sntp-simple branch 3 times, most recently from 56d2c76 to 1f47dea Compare April 25, 2019 10:52

pfalcon requested a review from rlubos April 25, 2019 10:53

vanti reviewed Apr 26, 2019

View reviewed changes

pfalcon force-pushed the sntp-simple branch from 1f47dea to 2b7ab36 Compare April 26, 2019 18:38

jukkar reviewed May 2, 2019

View reviewed changes

pfalcon force-pushed the sntp-simple branch from 2b7ab36 to e9f9090 Compare May 3, 2019 21:48

pfalcon requested a review from tbursztyka as a code owner May 3, 2019 21:48

pfalcon force-pushed the sntp-simple branch 3 times, most recently from 07c2c92 to aaf93ec Compare May 7, 2019 18:41

d3zd3z approved these changes May 7, 2019

View reviewed changes

tgorochowik reviewed May 8, 2019

View reviewed changes

pfalcon added 3 commits May 8, 2019 13:00

pfalcon force-pushed the sntp-simple branch from aaf93ec to 6ad8786 Compare May 8, 2019 10:14

pfalcon mentioned this pull request May 8, 2019

CMakeLists.txt: Link libkernel.a as whole-archive, like everything else #16021

Closed

jukkar approved these changes May 9, 2019

View reviewed changes

jukkar merged commit 22f1a29 into zephyrproject-rtos:master May 10, 2019

PiotrZierhoffer mentioned this pull request Jun 7, 2019

[RFC] Missing parts of libc required for CivetWeb #16683

Closed

net: sntp: Add "simple" one-shot API to query SNTP server #15602

net: sntp: Add "simple" one-shot API to query SNTP server #15602

Uh oh!

Conversation

pfalcon commented Apr 22, 2019

Uh oh!

pfalcon commented Apr 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pfalcon commented Apr 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfalcon commented May 7, 2019

Uh oh!

pfalcon commented May 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!