patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9612, Bugfix: sntp server IP address not...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #9612: Bugfix: sntp server IP address not always saved after DNS lookup

Submitter:  Tom Ferrin <tferrin>
Submitted:  Sat 31 Mar 2018 07:17:03 PM UTC
   
 
Category:  apps Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  goldsimon Open/Closed:  Closed
Planned Release:  None

Thu 05 Apr 2018 04:33:57 AM UTC, comment #5: 

(Sorry, hit submit prematurely.)

This bug fix is important for use with the reachability register added in patch https://savannah.nongnu.org/patch/?9581. With the addition of the reachability register and the reliable reporting of the IP addresses being used by the SNTP server, people/software can make intelligent choices about the NTP servers being used.

Tom Ferrin <tferrin>
Thu 05 Apr 2018 04:22:22 AM UTC, comment #4: 

Having access to the IP address(es) currently being used by the SNTP app is all about monitoring SNTP performance and is especially useful when multiple servers are configured. The DNS address cache doesn't come into play at all, except with respect to this bug fix. If the NTP server's address was in the cache, then the SNTP app correctly noted the address. If it was not in the cache, then the addr saved by SNTP was 0.0.0.0 and thus is not useful for monitoring SNTP performance differences among servers. SNTP always uses the IP address returned by the DNS server on each new NTP request, so the local copy is not relied on for proper operation of the protocol. Note that the default interval between time synchronization requests (SNTP_UPDATE_DELAY) is usually 60 minutes or more, so in an active network environment it is unlikely that the NTP server address will still be in the DNS cache and thus, prior to this bugfix, the address saved by SNTP was always 0.0.0.0. This bug fix only ensures that a valid address is saved by SNTP regardless of whether or not it was cached by the DNS server or had to be resolved through a DNS transaction.

Tom Ferrin <tferrin>
Wed 04 Apr 2018 01:16:02 PM UTC, comment #3: 

For me, it also sounds redundant, to cahce IP address in SNTP client code, because there is already cache in DNS client code.

But

 1) it may be helpfull in conjunction with NTP server pools. There may be some reason because of precision - it is better to stay with one server and not to jump to different server at every request.

 2) if you are implementing some monitoring, NTP server or so, you would like to know exact source of last succesful synchronization. Hostname is not enough for this - because of pools, was it over IPv4/IPv6?, etc.

We are using proprietary NTP client over LwIP so I don't know this SNTP client implementation, but in general we rely heavily on 2).

Jan Breuer <jan_breuer>
Wed 04 Apr 2018 12:38:36 PM UTC, comment #2: 

During these days I'm trying to setup the SNTP client in lwip, so I immediately noticed this bugfix.

Anyway I couldn't explain why sntp module needs to save the IP address, if SNTP_SERVER_DNS is set.

sntp_request() starts resetting the IP address (if SNTP_SERVER_DNS is set), so it appears to me .addr field is completely useless.  Why do we care to save the result of DNS query?

Maybe it is only useful if the application set the servername with sntp_setservername() and later tries to get the IP address with sntp_getserver().

Giuseppe Modugno <giusloq>
Tue 03 Apr 2018 07:17:27 PM UTC, comment #1: 

Hmm, isn't it the other way round and it should never store the IP address itself?
OTOH, it reads the IP address again next time in 'sntp_request()', so for informational purposes, storing the address might be OK...

Pushed, thanks for the patch.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 31 Mar 2018 07:17:03 PM UTC, original submission:  

When sntp resolves a server by name, it saves the resulting IP address in an array. But this only happens if the address was in the dns internal cache. If the dns module instead has to resolve the address by querying a name server, then the sntp module currently forgets to save the result. This one-line patch corrects that oversight.

Tom Ferrin <tferrin>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #43749:  sntp.c.patch added by tferrin (479B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jan_breuer (Posted a comment)
  • -email is unavailable- added by giusloq (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by tferrin (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-03 goldsimon StatusNone Done
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2018-03-31 tferrin Attached File- Added sntp.c.patch, #43749

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code