[{"id":2224,"web_url":"https://patchwork.libcamera.org/comment/2224/","msgid":"<20190711155417.GM5247@pendragon.ideasonboard.com>","date":"2019-07-11T15:54:17","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: logging: add\n\tlogging API for applications","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Jul 11, 2019 at 07:24:17PM +0900, Paul Elder wrote:\n> Currently the log file and the log level can only be set via environment\n> variables, but applications may also want to set the log file and the\n> log level at run time. Provide an API for this.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - add documentation\n> - actually set the log level\n> \n> include/libcamera/logging.h | 17 +++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/log.cpp | 54 +++++++++++++++++++++++++++++++++++\n> 3 files changed, 72 insertions(+)\n> create mode 100644 include/libcamera/logging.h\n> \n> diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h\n> new file mode 100644\n> index 0000000..47c5e49\n> --- /dev/null\n> +++ b/include/libcamera/logging.h\n> @@ -0,0 +1,17 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n\n2019\n\n> + *\n> + * log.h - Logging infrastructure\n\nlogging.h\n\n> + */\n> +#ifndef __LIBCAMERA_LOGGING_H__\n> +#define __LIBCAMERA_LOGGING_H__\n> +\n> +namespace libcamera {\n> +\n> +void logSetFile(const char *file);\n> +void logSetLevel(const char *category, const char *level);\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_LOGGING_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 972513f..920eb5f 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -9,6 +9,7 @@ libcamera_api = files([\n> 'geometry.h',\n> 'ipa/ipa_interface.h',\n> 'ipa/ipa_module_info.h',\n> + 'logging.h',\n> 'object.h',\n> 'request.h',\n> 'signal.h',\n> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp\n> index 0ba276e..dca4936 100644\n> --- a/src/libcamera/log.cpp\n> +++ b/src/libcamera/log.cpp\n> @@ -69,6 +69,9 @@ private:\n> \tvoid parseLogLevels();\n> \tstatic LogSeverity parseLogLevel(const std::string &level);\n> \n> +\tfriend void logSetFile(const char *file);\n> +\tfriend void logSetLevel(const char *category, const char *level);\n> +\n> \tfriend LogCategory;\n> \tvoid registerCategory(LogCategory *category);\n> \tvoid unregisterCategory(LogCategory *category);\n> @@ -80,6 +83,57 @@ private:\n> \tstd::ostream *output_;\n> };\n> \n> +/**\n> + * \\brief Set the log file\n> + * \\param[in] file\n\nThis is a public API so it should be properly documented. \n\n> + */\n> +void logSetFile(const char *file)\n> +{\n> +\tif (std::string(file).empty())\n> +\t\treturn;\n\n\tif (!file[0])\n\t\treturn;\n\nBut we don't usually check if mandatory parameters are null, so I would\nremove this check. Or maybe we should close the log file in that case\nand revert to stdout ?\n\n> +\n> +\tLogger *logger = Logger::instance();\n> +\tstd::ofstream &file_ = logger->file_;\n> +\tfile_.close();\n> +\tfile_.open(file);\n> +\tif (file_.good())\n> +\t\tlogger->output_ = &file_;\n\nShouldn't we catch error and avoid closing the log file in that case ?\nHow about\n\n\tstd::ofstream logFile(file);\n\tif (!logFile.good())\n\t\treturn -EINVAL;\n\n\tlogger->file_ = std::move(logFile);\n\tlogger->output_ = &logger->file_;\n\n\treturn 0;\n\n> +}\n> +\n> +/**\n> + * \\brief Set the log level\n> + * \\param[in] category Logging category\n> + * \\param[in] level Log level\n> + *\n> + * This function sets the log level of \\a category to \\a level.\n> + * \\a level should be one of the following strings:\n\ns/should/shall/\n\n> + * - \"DEBUG\"\n> + * - \"INFO\"\n> + * - \"WARN\"\n> + * - \"ERROR\"\n> + * - \"FATAL\"\n> + *\n> + * \"*\" is not a valid \\a category for this function.\n\ns/function/method/\n\n> + */\n> +void logSetLevel(const char *category, const char *level)\n> +{\n> +\tLogger *logger = Logger::instance();\n> +\tstd::string cat(category);\n> +\tstd::string lev(level);\n> +\n> +\t/* Both the category and the level must be specified. */\n> +\tif (cat.empty() || lev.empty())\n> +\t\treturn;\n> +\n\nSame here, I think you can remove this check.\n\n> +\tLogSeverity severity = Logger::parseLogLevel(lev);\n> +\tif (severity == LogInvalid)\n> +\t\treturn;\n> +\n> +\tfor (LogCategory *c : logger->categories_)\n> +\t\tif (!strcmp(c->name(), category))\n> +\t\t\tc->setSeverity(severity);\n> +}\n> +\n> /**\n> * \\brief Retrieve the logger instance\n> *","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7FCB460C3A\n\tfor ;\n\tThu, 11 Jul 2019 17:54:46 +0200 (CEST)","from pendragon.ideasonboard.com (softbank126163157105.bbtec.net\n\t[126.163.157.105])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E0E2B31C;\n\tThu, 11 Jul 2019 17:54:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1562860486;\n\tbh=M++OCgoZfNKUJcmTqWJhqXqH2FA6/I4SI7yF3lRvHnI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QfJT8DeBbzXYixCx3ohwome5jXOEOMNK6glVm1B5JkvqndtuUOSwvyLfE77mC0tNM\n\t6hajt6926RaO0XJrnaEnLwE1wbs1lbCOLNgkZAYdmr1Y4yx5h1nBZORaNCNJR8TvJs\n\tv/fV0nFyprMKbZmaTfcqPsJRy/7Rfn/cy/nRlysM=","Date":"Thu, 11 Jul 2019 18:54:17 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190711155417.GM5247@pendragon.ideasonboard.com>","References":"<20190711102418.29661-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190711102418.29661-1-paul.elder@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: logging: add\n\tlogging API for applications","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, 11 Jul 2019 15:54:46 -0000"}}]