[PATCH weston 2/8] shared: Add helpers to convert between protocol data and timespec
Alexandros Frantzis
alexandros.frantzis at collabora.com
Tue Dec 12 14:29:49 UTC 2017
On Tue, Dec 12, 2017 at 03:09:56PM +0200, Pekka Paalanen wrote:
> On Tue, 12 Dec 2017 14:43:11 +0200
> Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:
>
> > 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.
>
> Ok. I was thinking of saving in the number of local variables on every
> call site of timespec_to_proto().
>
> > > > 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).
>
> Why would the sign-extension be unwanted? I would expect it.
Hmm, I was thinking that on 32-bit time_t systems we would want to send
just the low 32-bits (and 0 for the high ones), but on second thought it
doesn't hurt to send a full 64-bit type representing the same value,
since the other side will just ignore the high bits anyway.
> > > 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.
>
> There would be no burden if we ensure our timestamps are always
> normalized, which is a precondition for the timespec sub/add etc.
> functions to operate without overflows. Therefore I'd push the
> normalize calls to not just the callers but the sites where the
> timestamps originate.
I am concerned that we are making timespec_to_proto unnecessarily
strict, and introducing more potential for runtime errors, but this is
certainly a viable path. I will update timespec_to_proto to have this
precondition.
> > 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.
>
> I think there are two different "normalized" here. One is struct
> timespec, another is what the protocol carries. Compared to struct
> timespec, the protocol has an additional requirement of non-negativity.
> I don't see that as leaking protocol encoding details - OTOH, we must
> not even attempt to send values the protocol cannot carry.
I think the difference it that passing a negative value is clearly an
invalid value, i.e., a logic error, whereas an unnormalized valid value is
actually a valid value with an unacceptable representation which we can
fix.
> > > - 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.
>
> How does client behaviour factor into it?
Sorry for the confusion, I was mostly referring to the behavior of
function callers , but I was also thinking in the background about
whether there could be a problem if this function ever got involved in
processing data sent from untrusted sources or clients (hence "client"
slipped in the wording). So, to rephrase, callers that use timestamps
that correspond to real current time should not be causing overflow
until 2038(32-bit)/ever(64-bits).
> I wouldn't expect anyone to handle timespec wrapping around, but for input
> events this can be fixed in the compositor by shifting the time base if
> even necessary.
>
> > 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.
>
> I wonder where time_t would still be 32-bit. I suppose we'll ignore the
> issue for now.
>
> > > > + 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,
> pq
Thanks,
Alexandros
More information about the wayland-devel
mailing list