[PATCH weston v5 03/14] libweston: add weston_debug API and implementation

[Daniel Stone]

Hi Pekka,

On Mon, 6 Aug 2018 at 12:16, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri, 20 Jul 2018 20:03:24 +0100 Daniel Stone <daniels at collabora.com> wrote:
> > +/** Format current time as a string
> > + * and append the debug scope name to it
> > + *
> > + * \param scope[in] debug scope.
> > + * \param buf[out] Buffer to store the string.
> > + * \param len Available size in the buffer in bytes.
> > + * \return \c buf
> > + *
> > + * Reads the current local wall-clock time and formats it into a string.
> > + * and append the debug scope name to it.
> > + * The string is nul-terminated, even if truncated.
> > + */
> > +WL_EXPORT char *
> > +weston_debug_scope_timestamp(struct weston_debug_scope *scope,
> > +                          char *buf, size_t len)
> > +{
> > +     struct timeval tv;
> > +     struct tm *bdt;
> > +     char string[128];
> > +     size_t ret = 0;
> > +
> > +     gettimeofday(&tv, NULL);
> > +
> > +     bdt = localtime(&tv.tv_sec);
> > +     if (bdt)
> > +             ret = strftime(string, sizeof string,
> > +                            "%Y-%m-%d %H:%M:%S", bdt);
> > +
> > +     if (ret > 0)
> > +             snprintf(buf, len, "[%s.%03ld][%s]", string,
> > +                      tv.tv_usec / 1000, scope->name);
> > +     else
> > +             snprintf(buf, len, "[?][%s]", scope->name);
> > +
> > +     return buf;
> > +}
>
> realized something when looking at the "log" debug scope patch:
> weston_debug_scope_timestamp() should probably be resilient against
> scope == NULL, all the other functions already are.
>
> weston_log gets initialized early, but the log debug scope gets
> initialized after the compositor. If something logs something between
> those two...

The users introduced in this patchset already check if the scope is
enabled, which bails out if the scope is NULL. Given that, and that I
can't see a sensible behaviour if the scope is NULL, I'm inclined to
add an assert(scope) at the top of the function as well as
documentation. Does that seem reasonable? Would assert(scope) be
better or assert(weston_debug_scope_is_enabled(scope))?

Cheers,
Daniel