[PATCH weston v2 1/2] compositor, backends: weston_compositor_read_presentation_clock

[Pekka Paalanen]

On Thu, 19 Mar 2015 11:29:52 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Thu, Mar 19, 2015 at 11:55:06AM +0200, Pekka Paalanen wrote:
> > On Wed, 18 Mar 2015 10:58:37 -0700
> > Bryce Harrington <bryce at osg.samsung.com> wrote:
> > 
> > > On Wed, Mar 18, 2015 at 03:27:21PM +0200, Pekka Paalanen wrote:
> > > > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > > > 
> > > If 0 time is returned, are we certain that's not going to screw up
> > > callers in some unexpected way?
> > 
> > We don't care too much. Reading the current time is always
> > somewhat uncertain and the calling code should be implicitly already
> > dealing with surprising values. Like in the follow-up I have a sanity
> > check to not delay for more than a second in the worst case.
> >
> > > If it's never meant to fail, and we don't want to propagate error codes
> > > from here, why not assert()?
> > 
> > It's not an assert() because there is a good chance it might not be a
> > bug in Weston's own code. If Weston is used in production, logging the
> > error and having a glitch is IMHO better than outright dying.
> 
> The man page of assert() indicates that for non-debug builds (NDEBUG not
> defined), the assert() macro does nothing.  Aside from logging an error
> message, it seems to do what you want?

Non-debug means NDEBUG is defined - does anyone actually use that?
It prevents running our test suite, too.

> > I think we are missing a bit of infrastructure here: an assert-like
> > macro that in debug builds aborts, but in production builds just logs
> > the error with as much detail as possible and attempts to continue.
> > It'd be good to also include rate limiting of logging.
> 
> Yes, I'd love to see this, I think it'd solve the need nicely.
> 
> > > Otherwise, seems to me like there'd be little harm propagating the error
> > > by returning false.  The current callers do ignore clock_gettime's
> > > return, but maybe some test someday will want to test conditions where
> > > clock_gettime wouldn't work properly...
> > 
> > Easy enough to change the return type when such a user appears. Until
> > then I don't see the point.
> > 
> > > 
> > > Apart from the error handling, rest looks fine:
> > > 
> > > Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
> > 
> > Is my explanation satisfactory enough?
> 
> Welll, it didn't change my mind.  :-) But the issue is pretty minor, and
> not worth blocking on if you want to land it as-is.

Ok, cool. If we get the infra, it's an easy grepping for 'warned' or
even weston_log and assert to find the places that will benefit.


Thanks,
pq