[{"id":30,"web_url":"https://patchwork.libcamera.org/comment/30/","msgid":"","date":"2018-12-06T14:24:01","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add initial logger","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 04/12/2018 22:11, Laurent Pinchart wrote:\n> The logger is based on the ostream API, allowing code to log messages in\n> a native way. It automatically logs the time stamp, severity level, file\n> name and line number.\n> \n> Many important features are missing, such as logging to file, logging\n> classes, and log filtering based on the severity level, file name and\n> class.\n> \n> Signed-off-by: Laurent Pinchart \n\nGreat - I like that it builds in the timestamping and file/line\ntracking, (which we might want to have as a verbosity option later).\n\nIt's better than not having anything... so lets get it in and let it\ndevelop as we go.\n\nAcked-by: Kieran Bingham \n\n\n> ---\n> src/libcamera/include/.keep_empty | 0\n> src/libcamera/include/log.h | 38 +++++++++++++++\n> src/libcamera/include/utils.h | 12 +++++\n> src/libcamera/log.cpp | 81 +++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 5 +-\n> 5 files changed, 135 insertions(+), 1 deletion(-)\n> delete mode 100644 src/libcamera/include/.keep_empty\n> create mode 100644 src/libcamera/include/log.h\n> create mode 100644 src/libcamera/include/utils.h\n> create mode 100644 src/libcamera/log.cpp\n> \n> diff --git a/src/libcamera/include/.keep_empty b/src/libcamera/include/.keep_empty\n> deleted file mode 100644\n> index e69de29bb2d1..000000000000\n> diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h\n> new file mode 100644\n> index 000000000000..76acd1520868\n> --- /dev/null\n> +++ b/src/libcamera/include/log.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * log.h - Logging infrastructure\n> + */\n> +#ifndef __LIBCAMERA_LOG_H__\n> +#define __LIBCAMERA_LOG_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +enum LogSeverity {\n> +\tLogInfo,\n> +\tLogWarning,\n> +\tLogError,\n> +};\n> +\n> +class LogMessage\n> +{\n> +public:\n> +\tLogMessage(const char *fileName, unsigned int line,\n> +\t\t LogSeverity severity);\n> +\tLogMessage(const LogMessage&) = delete;\n> +\t~LogMessage();\n> +\n> +\tstd::ostream& stream() { return msgStream; }\n> +\n> +private:\n> +\tstd::ostringstream msgStream;\n> +};\n> +\n> +#define LOG(severity) LogMessage(__FILE__, __LINE__, Log##severity).stream()\n> +\n> +};\n> +\n> +#endif /* __LIBCAMERA_LOG_H__ */\n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> new file mode 100644\n> index 000000000000..3ffa6f4ea591\n> --- /dev/null\n> +++ b/src/libcamera/include/utils.h\n> @@ -0,0 +1,12 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * utils.h - Miscellaneous utility functions\n> + */\n> +#ifndef __LIBCAMERA_UTILS_H__\n> +#define __LIBCAMERA_UTILS_H__\n> +\n> +#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> +\n> +#endif /* __LIBCAMERA_UTILS_H__ */\n> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> new file mode 100644\n> index 000000000000..18ccfa32d8b4\n> --- /dev/null\n> +++ b/src/libcamera/log.cpp\n> @@ -0,0 +1,81 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * log.h - Logging infrastructure\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file log.h\n> + * \\brief Logging infrastructure\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\enum LogSeverity\n> + * Log message severity\n> + * \\var Info\n> + * Informational message\n> + * \\var Warning\n> + * Warning message, signals a potential issue\n> + * \\var Error\n> + * Error message, signals an unrecoverable issue\n> + */\n> +\n> +/**\n> + * \\def LOG(severity)\n> + * \\brief Log a message\n> + *\n> + * Return an std::ostream reference to which a message can be logged using the\n> + * iostream API. The \\a severity controls whether the message is printed or\n> + * dropped, depending on the global log level.\n> + */\n> +\n> +static const char *log_severity_name(LogSeverity severity)\n> +{\n> +\tstatic const char * const names[] = {\n> +\t\t\"INFO\",\n> +\t\t\"WARN\",\n> +\t\t\" ERR\",\n> +\t};\n> +\n> +\tif ((unsigned int)severity < ARRAY_SIZE(names))\n> +\t\treturn names[severity];\n> +\telse\n> +\t\treturn \"UNKN\";\n> +}\n> +\n> +LogMessage::LogMessage(const char *fileName, unsigned int line,\n> +\t\t LogSeverity severity)\n> +{\n> +\t/* Log the timestamp, severity and file information. */\n> +\tstruct timespec timestamp;\n> +\tclock_gettime(CLOCK_MONOTONIC, ×tamp);\n> +\tmsgStream << \"[\" << timestamp.tv_sec / (60 * 60) << \":\"\n> +\t\t << std::setw(2) << (timestamp.tv_sec / 60) % 60 << \":\"\n> +\t\t << std::setw(2) << timestamp.tv_sec % 60 << \".\"\n> +\t\t << std::setw(9) << timestamp.tv_nsec << \"]\";\n> +\n> +\tmsgStream << \" \" << log_severity_name(severity);\n> +\tmsgStream << \" \" << basename(fileName) << \":\" << line << \" \";\n> +}\n> +\n> +LogMessage::~LogMessage()\n> +{\n> +\tmsgStream << std::endl;\n> +\n> +\tstd::string msg(msgStream.str());\n> +\tfwrite(msg.data(), msg.size(), 1, stderr);\n> +\tfflush(stderr);\n> +}\n> +\n> +};\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 07d9cd448342..fe38f8b2b5b4 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,4 +1,7 @@\n> -sources = ['main.cpp']\n> +sources = files([\n> + 'log.cpp',\n> + 'main.cpp',\n> +])\n> \n> includes = [\n> libcamera_includes,\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2CDD60B0B\n\tfor ;\n\tThu, 6 Dec 2018 15:24:04 +0100 (CET)","from [192.168.0.21]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58405595;\n\tThu, 6 Dec 2018 15:24:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1544106244;\n\tbh=G4Kko2HELupBgcxZw7yoxpeLQYIPFoiSdQBc2I1Ujfg=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=lAAB0dAtZNyqqydlAI27P9Uy9s5KoXcqCMF8LuzsYABhSQ5K4a5J2M3lw2sdtbnAP\n\tOXlKNBBieVyssh3cVQ5iTh9ks1pvsJO8+rlw0DPWjc0ukPnZV0tTIY7p80P9B2lglv\n\ti6cZJ6JVzPOHflwjsEt6+X/jBiAHADwFn7Xl+CjU=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20181204221123.571-1-laurent.pinchart@ideasonboard.com>\n\t<20181204221123.571-3-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\txsFNBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABzTBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT7CwYAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8tbOwU0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAHCwWUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 6 Dec 2018 14:24:01 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20181204221123.571-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add initial logger","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 06 Dec 2018 14:24:05 -0000"}},{"id":33,"web_url":"https://patchwork.libcamera.org/comment/33/","msgid":"<20181206171106.GC5597@w540>","date":"2018-12-06T17:11:06","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add initial logger","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n thanks for the patches\n\nOn Wed, Dec 05, 2018 at 12:11:22AM +0200, Laurent Pinchart wrote:\n> The logger is based on the ostream API, allowing code to log messages in\n> a native way. It automatically logs the time stamp, severity level, file\n> name and line number.\n>\n> Many important features are missing, such as logging to file, logging\n> classes, and log filtering based on the severity level, file name and\n> class.\n>\n\nHow do you expect to control filtering? Should we use a library-wide\ntag list to match on, which could be controlled through environment\nvariables? (such as GST_DEBUG is?)\n\nIn such a case, I wonder if it wouldn't be worth subclassing\nosstringstream and provide an overriden str() method that does the\nfiltering before calling the base class str() ?\n\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/include/.keep_empty | 0\n\n?\n\n> src/libcamera/include/log.h | 38 +++++++++++++++\n> src/libcamera/include/utils.h | 12 +++++\n> src/libcamera/log.cpp | 81 +++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 5 +-\n> 5 files changed, 135 insertions(+), 1 deletion(-)\n> delete mode 100644 src/libcamera/include/.keep_empty\n> create mode 100644 src/libcamera/include/log.h\n> create mode 100644 src/libcamera/include/utils.h\n> create mode 100644 src/libcamera/log.cpp\n>\n> diff --git a/src/libcamera/include/.keep_empty b/src/libcamera/include/.keep_empty\n> deleted file mode 100644\n> index e69de29bb2d1..000000000000\n> diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h\n> new file mode 100644\n> index 000000000000..76acd1520868\n> --- /dev/null\n> +++ b/src/libcamera/include/log.h\n> @@ -0,0 +1,38 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * log.h - Logging infrastructure\n> + */\n> +#ifndef __LIBCAMERA_LOG_H__\n> +#define __LIBCAMERA_LOG_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +enum LogSeverity {\n> +\tLogInfo,\n> +\tLogWarning,\n> +\tLogError,\n> +};\n> +\n> +class LogMessage\n> +{\n> +public:\n> +\tLogMessage(const char *fileName, unsigned int line,\n> +\t\t LogSeverity severity);\n> +\tLogMessage(const LogMessage&) = delete;\n> +\t~LogMessage();\n> +\n> +\tstd::ostream& stream() { return msgStream; }\n> +\n> +private:\n> +\tstd::ostringstream msgStream;\n> +};\n> +\n> +#define LOG(severity) LogMessage(__FILE__, __LINE__, Log##severity).stream()\n> +\n\nSo, each call to LOG will instantiate a new 'LogMessage' instance, I\nwonder if it wouldn't be better to have a log object global to the library\ninstead... I'm sure you have considerate that though..\n\n> +};\n> +\n> +#endif /* __LIBCAMERA_LOG_H__ */\n> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> new file mode 100644\n> index 000000000000..3ffa6f4ea591\n> --- /dev/null\n> +++ b/src/libcamera/include/utils.h\n> @@ -0,0 +1,12 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * utils.h - Miscellaneous utility functions\n> + */\n> +#ifndef __LIBCAMERA_UTILS_H__\n> +#define __LIBCAMERA_UTILS_H__\n> +\n> +#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> +\n> +#endif /* __LIBCAMERA_UTILS_H__ */\n> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> new file mode 100644\n> index 000000000000..18ccfa32d8b4\n> --- /dev/null\n> +++ b/src/libcamera/log.cpp\n> @@ -0,0 +1,81 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * log.h - Logging infrastructure\n\nThat's log.ccp :)\n\n> + */\n> +\n\nnit: in other files you don't have an empty line here\n\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file log.h\n\nditto\n\n> + * \\brief Logging infrastructure\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\enum LogSeverity\n> + * Log message severity\n> + * \\var Info\n> + * Informational message\n> + * \\var Warning\n> + * Warning message, signals a potential issue\n> + * \\var Error\n> + * Error message, signals an unrecoverable issue\n> + */\n\nShould we document types, enums etc in the cpp file, even if the\ndefinition is in the header file ?\n\n> +\n> +/**\n> + * \\def LOG(severity)\n> + * \\brief Log a message\n> + *\n> + * Return an std::ostream reference to which a message can be logged using the\n\nnit: ostringstream\n\n> + * iostream API. The \\a severity controls whether the message is printed or\n> + * dropped, depending on the global log level.\n> + */\n\nSame question: the macro is defined in the .h file. Documentation is\nuseful once it gets generated, for sure, but shall be useful when\nyou're only looking at the source code.\n\n> +\n> +static const char *log_severity_name(LogSeverity severity)\n> +{\n> +\tstatic const char * const names[] = {\n> +\t\t\"INFO\",\n> +\t\t\"WARN\",\n> +\t\t\" ERR\",\n ^ space\n\nOr is this intentional to have \"ERR\" messages stand out ?\n\n> +\t};\n> +\n> +\tif ((unsigned int)severity < ARRAY_SIZE(names))\n\nPlain old style C cast. I'm fine with that, this is very simple, but\nc++ style static_cast<> might be more appropriate\n\n\n> +\t\treturn names[severity];\n> +\telse\n> +\t\treturn \"UNKN\";\n> +}\n> +\n> +LogMessage::LogMessage(const char *fileName, unsigned int line,\n> +\t\t LogSeverity severity)\n> +{\n> +\t/* Log the timestamp, severity and file information. */\n> +\tstruct timespec timestamp;\n\nWhat about an empty line between variable declaration and code?\n\n> +\tclock_gettime(CLOCK_MONOTONIC, ×tamp);\n> +\tmsgStream << \"[\" << timestamp.tv_sec / (60 * 60) << \":\"\n> +\t\t << std::setw(2) << (timestamp.tv_sec / 60) % 60 << \":\"\n> +\t\t << std::setw(2) << timestamp.tv_sec % 60 << \".\"\n> +\t\t << std::setw(9) << timestamp.tv_nsec << \"]\";\n> +\n> +\tmsgStream << \" \" << log_severity_name(severity);\n> +\tmsgStream << \" \" << basename(fileName) << \":\" << line << \" \";\n> +}\n> +\n> +LogMessage::~LogMessage()\n> +{\n> +\tmsgStream << std::endl;\n> +\n> +\tstd::string msg(msgStream.str());\n> +\tfwrite(msg.data(), msg.size(), 1, stderr);\n> +\tfflush(stderr);\n> +}\n> +\n> +};\n\n/* namespace libcamera */\n\nThanks\n j\n\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 07d9cd448342..fe38f8b2b5b4 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,4 +1,7 @@\n> -sources = ['main.cpp']\n> +sources = files([\n> + 'log.cpp',\n> + 'main.cpp',\n> +])\n>\n> includes = [\n> libcamera_includes,\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 984C760B0C\n\tfor ;\n\tThu, 6 Dec 2018 18:11:09 +0100 (CET)","from w540 (2-224-242-101.ip172.fastwebnet.it [2.224.242.101])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 0E6D724000B;\n\tThu, 6 Dec 2018 17:11:08 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 6 Dec 2018 18:11:06 +0100","From":"jacopo mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181206171106.GC5597@w540>","References":"<20181204221123.571-1-laurent.pinchart@ideasonboard.com>\n\t<20181204221123.571-3-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"KDt/GgjP6HVcx58l\"","Content-Disposition":"inline","In-Reply-To":"<20181204221123.571-3-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add initial logger","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 06 Dec 2018 17:11:09 -0000"}},{"id":38,"web_url":"https://patchwork.libcamera.org/comment/38/","msgid":"<1759688.MaoLeofUuK@avalon>","date":"2018-12-10T22:04:12","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add initial logger","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thursday, 6 December 2018 19:11:06 EET jacopo mondi wrote:\n> Hi Laurent,\n> thanks for the patches\n> \n> On Wed, Dec 05, 2018 at 12:11:22AM +0200, Laurent Pinchart wrote:\n> > The logger is based on the ostream API, allowing code to log messages in\n> > a native way. It automatically logs the time stamp, severity level, file\n> > name and line number.\n> > \n> > Many important features are missing, such as logging to file, logging\n> > classes, and log filtering based on the severity level, file name and\n> > class.\n> \n> How do you expect to control filtering? Should we use a library-wide\n> tag list to match on, which could be controlled through environment\n> variables? (such as GST_DEBUG is?)\n\nI'm thinking about an environment variable indeed, likely with an API for \napplication to control logging manually.\n\n> In such a case, I wonder if it wouldn't be worth subclassing\n> osstringstream and provide an overriden str() method that does the\n> filtering before calling the base class str() ?\n\nI was thinking about implementing the filtering in the LogMessage destructor.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > \n> > src/libcamera/include/.keep_empty | 0\n> \n> ?\n\nThe file is deleted as the directory isn't empty anymore.\n\n> > src/libcamera/include/log.h | 38 +++++++++++++++\n> > src/libcamera/include/utils.h | 12 +++++\n> > src/libcamera/log.cpp | 81 +++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 5 +-\n> > 5 files changed, 135 insertions(+), 1 deletion(-)\n> > delete mode 100644 src/libcamera/include/.keep_empty\n> > create mode 100644 src/libcamera/include/log.h\n> > create mode 100644 src/libcamera/include/utils.h\n> > create mode 100644 src/libcamera/log.cpp\n> > \n> > diff --git a/src/libcamera/include/.keep_empty\n> > b/src/libcamera/include/.keep_empty deleted file mode 100644\n> > index e69de29bb2d1..000000000000\n> > diff --git a/src/libcamera/include/log.h b/src/libcamera/include/log.h\n> > new file mode 100644\n> > index 000000000000..76acd1520868\n> > --- /dev/null\n> > +++ b/src/libcamera/include/log.h\n> > @@ -0,0 +1,38 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * log.h - Logging infrastructure\n> > + */\n> > +#ifndef __LIBCAMERA_LOG_H__\n> > +#define __LIBCAMERA_LOG_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +enum LogSeverity {\n> > +\tLogInfo,\n> > +\tLogWarning,\n> > +\tLogError,\n> > +};\n> > +\n> > +class LogMessage\n> > +{\n> > +public:\n> > +\tLogMessage(const char *fileName, unsigned int line,\n> > +\t\t LogSeverity severity);\n> > +\tLogMessage(const LogMessage&) = delete;\n> > +\t~LogMessage();\n> > +\n> > +\tstd::ostream& stream() { return msgStream; }\n> > +\n> > +private:\n> > +\tstd::ostringstream msgStream;\n> > +};\n> > +\n> > +#define LOG(severity) LogMessage(__FILE__, __LINE__,\n> > Log##severity).stream()\n> > +\n> \n> So, each call to LOG will instantiate a new 'LogMessage' instance, I\n> wonder if it wouldn't be better to have a log object global to the library\n> instead... I'm sure you have considerate that though..\n\nI have :-) I actually have started with such an object, and then modified the \ndesign as it didn't really fit.\n\nThe core idea of this API is to define macros that will provide an ostream, to \nallow logging using the C++ stream API. The stream must be used on the stop, \nand the message must be flushed to the log backend automatically. Starting \nwith a Logger class that would contain an ostream, there was no easy way to \nimplement automatic flushing easily. That's why I decided to create a \nLogMessage class that would be instantiated for every LOG() call. The instance \nis temporary only, and serves as a wrapper around the ostream it contains. As \nthe instance is destroyed immediately after being used, the destructor serves \nas a convenient way to flush the log message.\n\n> > +};\n> > +\n> > +#endif /* __LIBCAMERA_LOG_H__ */\n> > diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h\n> > new file mode 100644\n> > index 000000000000..3ffa6f4ea591\n> > --- /dev/null\n> > +++ b/src/libcamera/include/utils.h\n> > @@ -0,0 +1,12 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * utils.h - Miscellaneous utility functions\n> > + */\n> > +#ifndef __LIBCAMERA_UTILS_H__\n> > +#define __LIBCAMERA_UTILS_H__\n> > +\n> > +#define ARRAY_SIZE(a)\t(sizeof(a) / sizeof(a[0]))\n> > +\n> > +#endif /* __LIBCAMERA_UTILS_H__ */\n> > diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> > new file mode 100644\n> > index 000000000000..18ccfa32d8b4\n> > --- /dev/null\n> > +++ b/src/libcamera/log.cpp\n> > @@ -0,0 +1,81 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * log.h - Logging infrastructure\n> \n> That's log.ccp :)\n\nOr even log.cpp. I'll fix that.\n\n> \n> > + */\n> > +\n> \n> nit: in other files you don't have an empty line here\n\nI found an empty line here in .cpp files easier to read, but that's not a big \ndeal, we can remove it if you think that's better. Or add an empty line in .h \nfiles.\n\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"utils.h\"\n> > +\n> > +/**\n> > + * \\file log.h\n> \n> ditto\n\nWill fix too.\n\n> > + * \\brief Logging infrastructure\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\enum LogSeverity\n> > + * Log message severity\n> > + * \\var Info\n> > + * Informational message\n> > + * \\var Warning\n> > + * Warning message, signals a potential issue\n> > + * \\var Error\n> > + * Error message, signals an unrecoverable issue\n> > + */\n> \n> Should we document types, enums etc in the cpp file, even if the\n> definition is in the header file ?\n\nI think so. Documentation in the .h file makes the header harder to read if \nyou want to get an overview of the API. Beside, I think we should be \nconsistent and document everything in the .h file or the .cpp file, and as \nmost of the documentation should relate (for most classes) to member functions \nthat we decided to document in the .cpp file, let's document everything there.\n\n> > +\n> > +/**\n> > + * \\def LOG(severity)\n> > + * \\brief Log a message\n> > + *\n> > + * Return an std::ostream reference to which a message can be logged\n> > using the\n> \n> nit: ostringstream\n\n std::ostream& stream() { return msgStream; }\n\nIt's an std::ostream. The fact that the returned ostream is an ostringstream \nis an implementation detail.\n\n> > + * iostream API. The \\a severity controls whether the message is printed\n> > or\n> > + * dropped, depending on the global log level.\n> > + */\n> \n> Same question: the macro is defined in the .h file. Documentation is\n> useful once it gets generated, for sure, but shall be useful when\n> you're only looking at the source code.\n\nSame answer :-)\n\n> > +\n> > +static const char *log_severity_name(LogSeverity severity)\n> > +{\n> > +\tstatic const char * const names[] = {\n> > +\t\t\"INFO\",\n> > +\t\t\"WARN\",\n> > +\t\t\" ERR\",\n> \n> ^ space\n> \n> Or is this intentional to have \"ERR\" messages stand out ?\n\nIt's not to make them stand out, but to align all messages the same way. I \ncould put the space after ERR if you prefer :-)\n\n> > +\t};\n> > +\n> > +\tif ((unsigned int)severity < ARRAY_SIZE(names))\n> \n> Plain old style C cast. I'm fine with that, this is very simple, but\n> c++ style static_cast<> might be more appropriate\n\nGood point. The cast might actually not be needed, as g++ converts severity to \nan integer type. I haven't been able to find a guarantee in the C++ \nspecification that the integer would be unsigned though, so I'll use a \nstatic_cast<>.\n\n> > +\t\treturn names[severity];\n> > +\telse\n> > +\t\treturn \"UNKN\";\n> > +}\n> > +\n> > +LogMessage::LogMessage(const char *fileName, unsigned int line,\n> > +\t\t LogSeverity severity)\n> > +{\n> > +\t/* Log the timestamp, severity and file information. */\n> > +\tstruct timespec timestamp;\n> \n> What about an empty line between variable declaration and code?\n\nYou won't like this, but in C++ the functions aren't split anymore between \nvariables and code. Variables get declared along with the code that uses them. \nI don't want to push that rule too far though, as it sometimes hinder \nreadability, but in this case I think it is fine.\n\n> > +\tclock_gettime(CLOCK_MONOTONIC, ×tamp);\n> > +\tmsgStream << \"[\" << timestamp.tv_sec / (60 * 60) << \":\"\n> > +\t\t << std::setw(2) << (timestamp.tv_sec / 60) % 60 << \":\"\n> > +\t\t << std::setw(2) << timestamp.tv_sec % 60 << \".\"\n> > +\t\t << std::setw(9) << timestamp.tv_nsec << \"]\";\n> > +\n> > +\tmsgStream << \" \" << log_severity_name(severity);\n> > +\tmsgStream << \" \" << basename(fileName) << \":\" << line << \" \";\n> > +}\n> > +\n> > +LogMessage::~LogMessage()\n> > +{\n> > +\tmsgStream << std::endl;\n> > +\n> > +\tstd::string msg(msgStream.str());\n> > +\tfwrite(msg.data(), msg.size(), 1, stderr);\n> > +\tfflush(stderr);\n> > +}\n> > +\n> > +};\n> \n> /* namespace libcamera */\n\nWill fix (and in log.h too), and will remove the ; after the } as it's not \nrequired.\n\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 07d9cd448342..fe38f8b2b5b4 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,4 +1,7 @@\n> > -sources = ['main.cpp']\n> > +sources = files([\n> > + 'log.cpp',\n> > + 'main.cpp',\n> > +])\n> > \n> > includes = [\n> > libcamera_includes,","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4730460B0B\n\tfor ;\n\tMon, 10 Dec 2018 23:03:30 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C844654B;\n\tMon, 10 Dec 2018 23:03:29 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1544479410;\n\tbh=Gof5hC3UlfW4aNeBMfVrUcrPTPBt3e19Zd+lqNIXCFc=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=OHl2AOEMUwZSyzIFxME+0zIBKFn3m48JItr8PNbjYFFCFi2rXueLY/ajI74AHFgrd\n\tqQAKowgv278Zypp0OiH1kADZElphYRM7bnB9VsP/AntxiFqEWUjpFHSGPdEy18ziL4\n\tv6T9wqzXanI/TqrgLD9WwTyDvsaVR7YmteSrVVHs=","From":"Laurent Pinchart ","To":"jacopo mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 11 Dec 2018 00:04:12 +0200","Message-ID":"<1759688.MaoLeofUuK@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181206171106.GC5597@w540>","References":"<20181204221123.571-1-laurent.pinchart@ideasonboard.com>\n\t<20181204221123.571-3-laurent.pinchart@ideasonboard.com>\n\t<20181206171106.GC5597@w540>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: Add initial logger","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 10 Dec 2018 22:03:30 -0000"}}]