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:[email protected]] Sent: Mittwoch, 30. August 2017 14:25 To: Devadoss, Maniraj (RBEI/ECF3; ADITG/SWG) Cc: [email protected] Subject: Re: [PATCH weston 1/8] libweston: add weston_debug API and implementation On Thu, 24 Aug 2017 16:16:15 +0200 Maniraj Devadoss <[email protected]> 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 <[email protected]> > > 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 <[email protected]> > --- > 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 _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
