lfpinit() signed or unsigned?

[Eric S. Raymond]

Achim Gratz <Stromeko at nexgo.de>:
> Also, proper review might avoid "improvements"
> that really aren't, like this:
> 
> --8<---------------cut here---------------start------------->8---
> @@ -13,11 +13,22 @@
>   * NTP uses two fixed point formats.  The first (l_fp) is the "long"
>   * format and is 64 bits wide with the decimal between bits 31 and 32.
>   * This is used for time stamps in the NTP packet header (in network
> - * byte order) and for internal computations of offsets (in local host
> - * byte order). We use the same structure for both signed and unsigned
> - * values, which is a big hack but saves rewriting all the operators
> - * twice. Just to confuse this, we also sometimes just carry the
> - * fractional part in calculations, in both signed and unsigned forms.
> + * byte order).  It is defined in RFC 5905 in Section 6 (Data Types).
> + *
> + * The integral part is unsigned seconds since 0 h 1 January 1900 UTC.
> + * It will overflow in 2036.
> + *
> + * The fractional part is jusr float seconds divided by 32.  Each LSB
> + * is 232 pico seconds.
> + *
> + * For internal computations of offsets (in local host byte order)
> + * the same structure is used, but signed, to allow for positive and
> + * negative offsets. We use the same structure for both signed and
> + * unsigned values, which is a big hack but saves rewriting all the
> + * operators twice. Just to confuse this, we also sometimes just carry
> + * the fractional part in calculations, in both signed and unsigned
> + * forms.
> + *
>   * Anyway, an l_fp looks like:
>   *
>   *    0			  1		      2			  3
> --8<---------------cut here---------------end--------------->8---
> 
> Again, l_fp is no structure anymore.  It doesn't really consist of an
> integral and fractional part either now that it is no longer a
> structure, it simply is 64bits in a unit of seconds scaled by 2e-32.
> That it was a structure consisting of two 32bit parts is an
> implementation detail of how the older code represented NTP time.
> 
> There's no "float seconds" and especially none that are "divided by 32"
> (which would be a right shift by 5).  The LSB unit would be 233ps if you
> round to picosecond precision.
> 
> The offset computations don't really use the same structure or even the
> same type, but they use the same time unit and are indeed signed.  The
> hack the old code used was to not define a separate structure for that
> and just stuff the offests and timestamps into the same structure with
> oblivion.  There is absolutely no way to treat the lower 32bit
> (fractional seconds) directly as a signed type unless you first
> determine the sign or direction of the offset and branch based on that
> information.

This is all correct.  I have revised the comment as follows:

 * NTP uses two fixed point formats.
 *
 * The first (l_fp) is the "long" format and is 64 bits wide in units
 * of 1/2e32 seconds (which is between 232 and 233 decimal
 * picoseconds).  The zero value signifies the zero date of the
 * current NTP era; era zero began on the date 1900-00-00T00:00:00 in
 * proleptic UTC (leap second correction was not introduced until
 * 1972).
 *
 * The long format is used for timestamps in the NTP packet header (in
 * network byte order).  It is defined in RFC 5905 in Section 6 (Data
 * Types). In the on-the-wire context, it is always unsigned.
 *
 * When it is convenient to compute in float seconds, this type can
 * be interpreted as a fixed-point float with the radix point between
 * bits 31 and 32. This is why there are macros to extract the low and
 * high halves.
 *
 * Internally, this type is sometimes used for time offsets.  In that 
 * context it is interpreted as signed and can only express offsets
 * up to a half cycle. Offsets are normally much, much smaller than that;
 * for an offset to have a value even as large as 1 second would be 
 * highly unusual.

> Here's another one:
> --8<---------------cut here---------------start------------->8---
> @@ -114,6 +114,22 @@ normalize_tspec(
>  	return x;
>  }
>  
> +/* convert a double to a rounded and normalized timespec */
> +static inline struct timespec
> +d_to_tspec(
> +	double d
> +	)
> +{
> +	struct timespec	x;
> +
> +	d += 0.5e-9;	/* round on LSB */
> +	x.tv_sec = (time_t) d;
> +	x.tv_nsec = (long)((d - (double)x.tv_sec) * 1e9);
> +
> +        /* unlikely, but ensure it is normalized */
> +	return normalize_tspec(x);
> +}
> +
>  /* x = a + b */
>  static inline struct timespec
>  add_tspec(
> --8<---------------cut here---------------end--------------->8---
> 
> Sprinkling magical constants throughout the code like that isn't going
> to make it easier to maintain (especially when you consider that there's
> a myriad ways of writing the same number).  There's a reason NANOSECONDS
> gets defined at the top of that file, even though it's unlikely that the
> unit of time will change in timespec.  So (0.5/NANOSECONDS) and (… *
> NANOSECONDS) would be much easier to find in the event that later on
> someone wanted to bump the filed sizes up and scale to attoseconds.

This is also correct.  And everyone here should know better than to introduce
magic constants!
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Please consider contributing to my Patreon page at https://www.patreon.com/esr
so I can keep the invisible wheels of the Internet turning. Give generously -
the civilization you save might be your own.