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

[Bryce Harrington]

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?
 
> 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.

Bryce