[libcamera-devel] [PATCH 2/3] libcamera: Add initial logger
jacopo mondi
jacopo at jmondi.org
Thu Dec 6 18:11:06 CET 2018
Hi Laurent,
thanks for the patches
On Wed, Dec 05, 2018 at 12:11:22AM +0200, Laurent Pinchart wrote:
> The logger is based on the ostream API, allowing code to log messages in
> a native way. It automatically logs the time stamp, severity level, file
> name and line number.
>
> Many important features are missing, such as logging to file, logging
> classes, and log filtering based on the severity level, file name and
> class.
>
How do you expect to control filtering? Should we use a library-wide
tag list to match on, which could be controlled through environment
variables? (such as GST_DEBUG is?)
In such a case, I wonder if it wouldn't be worth subclassing
osstringstream and provide an overriden str() method that does the
filtering before calling the base class str() ?
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/libcamera/include/.keep_empty | 0
?
> src/libcamera/include/log.h | 38 +++++++++++++++
> src/libcamera/include/utils.h | 12 +++++
> src/libcamera/log.cpp | 81 +++++++++++++++++++++++++++++++
> src/libcamera/meson.build | 5 +-
> 5 files changed, 135 insertions(+), 1 deletion(-)
> delete mode 100644 src/libcamera/include/.keep_empty
> create mode 100644 src/libcamera/include/log.h
> create mode 100644 src/libcamera/include/utils.h
> create mode 100644 src/libcamera/log.cpp
>
> diff --git a/src/libcamera/include/.keep_empty b/src/libcamera/include/.keep_empty
> deleted file mode 100644
> index e69de29bb2d1..000000000000
> diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h
> new file mode 100644
> index 000000000000..76acd1520868
> --- /dev/null
> +++ b/src/libcamera/include/log.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * log.h - Logging infrastructure
> + */
> +#ifndef __LIBCAMERA_LOG_H__
> +#define __LIBCAMERA_LOG_H__
> +
> +#include <sstream>
> +
> +namespace libcamera {
> +
> +enum LogSeverity {
> + LogInfo,
> + LogWarning,
> + LogError,
> +};
> +
> +class LogMessage
> +{
> +public:
> + LogMessage(const char *fileName, unsigned int line,
> + LogSeverity severity);
> + LogMessage(const LogMessage&) = delete;
> + ~LogMessage();
> +
> + std::ostream& stream() { return msgStream; }
> +
> +private:
> + std::ostringstream msgStream;
> +};
> +
> +#define LOG(severity) LogMessage(__FILE__, __LINE__, Log##severity).stream()
> +
So, each call to LOG will instantiate a new 'LogMessage' instance, I
wonder if it wouldn't be better to have a log object global to the library
instead... I'm sure you have considerate that though..
> +};
> +
> +#endif /* __LIBCAMERA_LOG_H__ */
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> new file mode 100644
> index 000000000000..3ffa6f4ea591
> --- /dev/null
> +++ b/src/libcamera/include/utils.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * utils.h - Miscellaneous utility functions
> + */
> +#ifndef __LIBCAMERA_UTILS_H__
> +#define __LIBCAMERA_UTILS_H__
> +
> +#define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
> +
> +#endif /* __LIBCAMERA_UTILS_H__ */
> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> new file mode 100644
> index 000000000000..18ccfa32d8b4
> --- /dev/null
> +++ b/src/libcamera/log.cpp
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * log.h - Logging infrastructure
That's log.ccp :)
> + */
> +
nit: in other files you don't have an empty line here
> +#include <cstdio>
> +#include <ctime>
> +#include <iomanip>
> +#include <string.h>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +/**
> + * \file log.h
ditto
> + * \brief Logging infrastructure
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \enum LogSeverity
> + * Log message severity
> + * \var Info
> + * Informational message
> + * \var Warning
> + * Warning message, signals a potential issue
> + * \var Error
> + * Error message, signals an unrecoverable issue
> + */
Should we document types, enums etc in the cpp file, even if the
definition is in the header file ?
> +
> +/**
> + * \def LOG(severity)
> + * \brief Log a message
> + *
> + * Return an std::ostream reference to which a message can be logged using the
nit: ostringstream
> + * iostream API. The \a severity controls whether the message is printed or
> + * dropped, depending on the global log level.
> + */
Same question: the macro is defined in the .h file. Documentation is
useful once it gets generated, for sure, but shall be useful when
you're only looking at the source code.
> +
> +static const char *log_severity_name(LogSeverity severity)
> +{
> + static const char * const names[] = {
> + "INFO",
> + "WARN",
> + " ERR",
^ space
Or is this intentional to have "ERR" messages stand out ?
> + };
> +
> + if ((unsigned int)severity < ARRAY_SIZE(names))
Plain old style C cast. I'm fine with that, this is very simple, but
c++ style static_cast<> might be more appropriate
> + return names[severity];
> + else
> + return "UNKN";
> +}
> +
> +LogMessage::LogMessage(const char *fileName, unsigned int line,
> + LogSeverity severity)
> +{
> + /* Log the timestamp, severity and file information. */
> + struct timespec timestamp;
What about an empty line between variable declaration and code?
> + clock_gettime(CLOCK_MONOTONIC, ×tamp);
> + msgStream << "[" << timestamp.tv_sec / (60 * 60) << ":"
> + << std::setw(2) << (timestamp.tv_sec / 60) % 60 << ":"
> + << std::setw(2) << timestamp.tv_sec % 60 << "."
> + << std::setw(9) << timestamp.tv_nsec << "]";
> +
> + msgStream << " " << log_severity_name(severity);
> + msgStream << " " << basename(fileName) << ":" << line << " ";
> +}
> +
> +LogMessage::~LogMessage()
> +{
> + msgStream << std::endl;
> +
> + std::string msg(msgStream.str());
> + fwrite(msg.data(), msg.size(), 1, stderr);
> + fflush(stderr);
> +}
> +
> +};
/* namespace libcamera */
Thanks
j
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 07d9cd448342..fe38f8b2b5b4 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,4 +1,7 @@
> -sources = ['main.cpp']
> +sources = files([
> + 'log.cpp',
> + 'main.cpp',
> +])
>
> includes = [
> libcamera_includes,
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181206/ee9530a1/attachment.sig>
More information about the libcamera-devel
mailing list