[PATCH weston 2/8] shared: Add helpers to convert between protocol data and timespec

[Alexandros Frantzis]

On Tue, Dec 12, 2017 at 12:09:59PM +0200, Pekka Paalanen wrote:
> On Mon,  4 Dec 2017 15:34:02 +0200
> Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
> 
> > Add helpers to safely convert between struct timespec values and
> > tv_sec_hi, tv_sec_lo, tv_nsec triplets used for sending high-resolution
> > timestamp data over the wayland protocol. Replace existing conversion
> > code with the helper functions.
> > 
> > Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> > ---
> >  clients/presentation-shm.c |  9 +------
> >  libweston/compositor.c     |  9 ++++---
> >  shared/timespec-util.h     | 39 +++++++++++++++++++++++++++
> >  tests/presentation-test.c  |  9 +------
> >  tests/timespec-test.c      | 66 ++++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 112 insertions(+), 20 deletions(-)
> 
> Hi,

Hi,

> 
> I have questions below that will have implications to the protocol
> extension spec as well. I would like to require the time values in
> protocol to be normalized.

Ack.

> > 
> > diff --git a/clients/presentation-shm.c b/clients/presentation-shm.c
> > index c9fb66cc..d6a939e5 100644
> > --- a/clients/presentation-shm.c
> > +++ b/clients/presentation-shm.c
> > @@ -39,6 +39,7 @@
> >  #include <wayland-client.h>
> >  #include "shared/helpers.h"
> >  #include "shared/zalloc.h"
> > +#include "shared/timespec-util.h"
> >  #include "shared/os-compatibility.h"
> >  #include "presentation-time-client-protocol.h"
> >  
> > @@ -383,14 +384,6 @@ timespec_to_ms(const struct timespec *ts)
> >  	return (uint32_t)ts->tv_sec * 1000 + ts->tv_nsec / 1000000;
> >  }
> >  
> > -static void
> > -timespec_from_proto(struct timespec *tm, uint32_t tv_sec_hi,
> > -		    uint32_t tv_sec_lo, uint32_t tv_nsec)
> > -{
> > -	tm->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
> > -	tm->tv_nsec = tv_nsec;
> > -}
> > -
> >  static int
> >  timespec_diff_to_usec(const struct timespec *a, const struct timespec *b)
> >  {
> > diff --git a/libweston/compositor.c b/libweston/compositor.c
> > index 7d7a17ed..083664fd 100644
> > --- a/libweston/compositor.c
> > +++ b/libweston/compositor.c
> > @@ -341,7 +341,9 @@ weston_presentation_feedback_present(
> >  {
> >  	struct wl_client *client = wl_resource_get_client(feedback->resource);
> >  	struct wl_resource *o;
> > -	uint64_t secs;
> > +	uint32_t tv_sec_hi;
> > +	uint32_t tv_sec_lo;
> > +	uint32_t tv_nsec;
> 
> A suggestion: how about introducing
> 
> struct timespec_proto {
> 	uint32_t sec_hi;
> 	uint32_t sec_lo;
> 	uint32_t nsec;
> };
> 
> and using that in timespec_to_proto()?
> 
> (Not useful for timespec_from_proto() because the three variables are
> already declared.)

I am not opposed to introducing a struct timespec_proto in general, but
using it in only one of the two functions feels inconsistent.

> >  	wl_resource_for_each(o, &output->resource_list) {
> >  		if (wl_resource_get_client(o) != client)
> > @@ -350,10 +352,9 @@ weston_presentation_feedback_present(
> >  		wp_presentation_feedback_send_sync_output(feedback->resource, o);
> >  	}
> >  
> > -	secs = ts->tv_sec;
> > +	timespec_to_proto(ts, &tv_sec_hi, &tv_sec_lo, &tv_nsec);
> >  	wp_presentation_feedback_send_presented(feedback->resource,
> > -						secs >> 32, secs & 0xffffffff,
> > -						ts->tv_nsec,
> > +						tv_sec_hi, tv_sec_lo, tv_nsec,
> >  						refresh_nsec,
> >  						seq >> 32, seq & 0xffffffff,
> >  						flags | feedback->psf_flags);
> > diff --git a/shared/timespec-util.h b/shared/timespec-util.h
> > index a10edf5b..c734accd 100644
> > --- a/shared/timespec-util.h
> > +++ b/shared/timespec-util.h
> > @@ -175,6 +175,30 @@ timespec_to_usec(const struct timespec *a)
> >  	return (int64_t)a->tv_sec * 1000000 + a->tv_nsec / 1000;
> >  }
> >  
> > +/* Convert timespec to protocol data
> > + *
> > + * \param a timespec
> > + * \param tv_sec_hi[out] the high bytes of the seconds part
> > + * \param tv_sec_lo[out] the low bytes of the seconds part
> > + * \param tv_nsec[out] the nanoseconds part
> > + *
> > + * The timespec is normalized before being converted to protocol data.
> > + */
> > +static inline void
> > +timespec_to_proto(const struct timespec *a, uint32_t *tv_sec_hi,
> > +                  uint32_t *tv_sec_lo, uint32_t *tv_nsec)
> > +{
> > +	struct timespec r;
> > +
> > +	timespec_normalize(&r, a);
> > +
> > +	/* We check the size of tv_sec, so that we shift only if the size
> > +	 * is 64-bits, in order to avoid sign extension on 32-bit systems. */
> > +	*tv_sec_hi = sizeof(r.tv_sec) == 8 ? (int64_t)r.tv_sec >> 32 : 0;
> 
> How about just casting tv_sec to uint64_t before shifting and masking
> it to 32-bit pieces? I think that would be more obvious to read.

We can only do this safely if tv_sec is non-negative (due to
int32_t->uint64_t sign extension when time_t is 32 bits), which
I agree is a sensible pre-condition/assertion to have (see below).

> I think it would be better to assert() that the timespec is already:
> 
> - normalized, because otherwise nsec might overflow in a computation,
>   and

If we make normalized input a pre-condition of timespec_to_proto, we are
essentially moving the burden to the caller, which in most cases should
call timespec_normalize themselves before calling timespec_to_proto. I
am not sure if we should burden the callers with this and introduce the
possibility of runtime errors.

Also, I like the idea that the callers don't need to care about protocol
encoding details, and just let timespec_to_proto do the conversion
properly.

> - non-negative, because the protocol cannot carry negative
>   timestamps.

Makes sense.

> 
> > +	*tv_sec_lo = r.tv_sec;
> > +	*tv_nsec = r.tv_nsec;
> > +}
> > +
> >  /* Convert nanoseconds to timespec
> >   *
> >   * \param a timespec
> > @@ -209,6 +233,21 @@ timespec_from_msec(struct timespec *a, int64_t b)
> >  	timespec_from_nsec(a, b * 1000000);
> >  }
> >  
> > +/* Convert protocol data to timespec
> > + *
> > + * \param a[out] timespec
> > + * \param tv_sec_hi the high bytes of seconds part
> > + * \param tv_sec_lo the low bytes of seconds part
> > + * \param tv_nsec the nanoseconds part
> > + */
> > +static inline void
> > +timespec_from_proto(struct timespec *a, uint32_t tv_sec_hi,
> > +                    uint32_t tv_sec_lo, uint32_t tv_nsec)
> > +{
> > +	a->tv_sec = ((uint64_t)tv_sec_hi << 32) + tv_sec_lo;
> 
> How to handle overflows in tv_sec would be a good question...

A good question indeed. Overflow shouldn't be a problem for well
behaving clients (if we are talking about real-time timestamps then not
until 2038 for 32-bits and not ever for 64-bits). The question is how to
deal with misbehaving and/or malicious clients.

There are two aspects to this. The first is 64-bit to 32-bit overflow on
32-bit systems, which is currently handled by implicitly dropping the
high 32-bits. The second is unsigned to signed overflow, which currently
produces negative timestamps. One way to deal with this is to clamp to
the largest positive value if we ever get a negative value. Another way
is to force the sign bit to 0, essentially making negative values cycle
back to non-negative.

> > +	a->tv_nsec = tv_nsec;
> > +}
> 
> Btw. refactoring code to introduce timespec_from_proto() with no
> functional changes should probably be a patch of its own.

Sure, I will split this in v2.

Thanks,
Alexandros