[PATCH weston 1/8] libweston: add weston_debug API and implementation

[Devadoss, Maniraj (RBEI/ECF3; ADITG/SWG)]

Hello Pekka,

Thanks for your review comments. As I was on vacation I couldn't reply on time. I will do the modifications and upstream the patches.

Best Regards,
Maniraj D.

-----Original Message-----
From: Pekka Paalanen [mailto:ppaalanen at gmail.com] 
Sent: Mittwoch, 30. August 2017 14:25
To: Devadoss, Maniraj (RBEI/ECF3; ADITG/SWG)
Cc: wayland-devel at lists.freedesktop.org
Subject: Re: [PATCH weston 1/8] libweston: add weston_debug API and implementation

On Thu, 24 Aug 2017 16:16:15 +0200
Maniraj Devadoss <external.mdevadoss at de.adit-jv.com> wrote:

> weston_debug is both a libweston API for relaying debugging messages, 
> and the compositor-debug wayland protocol implementation for accessing 
> those debug messages from a Wayland client.
> 
> weston_debug_compositor_{create,destroy}() are private API, hence not 
> exported.
> 
> Signed-off-by: Pekka Paalanen <pq at iki.fi>
> 
> use the compositor-debug protocol from wayland-protocols

Hi,

you forgot to mention the changes I point out below.

There should also be a note that this patch depends on an unreleased version of wayland-protocols. I think we might want a wayland-protocols release before merging this patch upstream, so that we can bump the wayland-protocols version requirement in this patch.

> 
> Signed-off-by: Maniraj Devadoss <external.mdevadoss at de.adit-jv.com>
> ---
>  Makefile.am              |   6 +
>  libweston/compositor.c   |   5 +
>  libweston/compositor.h   |   8 +
>  libweston/weston-debug.c | 755 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  libweston/weston-debug.h | 107 +++++++
>  5 files changed, 881 insertions(+)
>  create mode 100644 libweston/weston-debug.c  create mode 100644 
> libweston/weston-debug.h
> 


> diff --git a/libweston/weston-debug.c b/libweston/weston-debug.c new 
> file mode 100644 index 0000000..d7a026b
> --- /dev/null
> +++ b/libweston/weston-debug.c

> +/** 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

As can be seen from this left-over documentation, the previous version
of this function returned the argument 'buf' as is. You said that using
the return value from this function instead of the passed-in argument
in the callers causes a crash. I would really like to understand why
that is before accepting this change. I cannot find a reason for it to
fail.

> + *
> + * 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 void
> +weston_debug_scope_timestamp(struct weston_debug_scope *scope,
> +			     char *buf, size_t len)

Adding the scope name here is a fine change, but is another thing
forgotten from the commit message.

> +{
> +	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;

This produces a warning:

/home/pq/git/weston/libweston/weston-debug.c: In function ‘weston_debug_scope_timestamp’:
/home/pq/git/weston/libweston/weston-debug.c:754:2: warning: ‘return’ with a value, in function returning void

Introducing a compiler warning is considered a regression.

> +}

Other than the issues mentioned, the changes look good.


Thanks,
pq