[{"id":31745,"web_url":"https://patchwork.libcamera.org/comment/31745/","msgid":"<20241015102225.GE5682@pendragon.ideasonboard.com>","date":"2024-10-15T10:22:25","subject":"Re: [PATCH 2/2] libcamera: log: add logTargetCros for ChromeOS logs","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey, Han-Lin,\n\nThank you for the patch.\n\nOn Mon, Oct 14, 2024 at 08:13:55AM +0000, Harvey Yang wrote:\n> From: Han-Lin Chen \n> \n> ChromeOS logging requires that the logs export to both syslog and\n> std::cerr. Add a logTargetCros for the specific case.\n> \n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/logging.h | 1 +\n> src/libcamera/base/log.cpp | 61 +++++++++++++++++++++++++++++++------\n> 2 files changed, 53 insertions(+), 9 deletions(-)\n> \n> diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h\n> index e1c6341ce..b88acf23b 100644\n> --- a/include/libcamera/logging.h\n> +++ b/include/libcamera/logging.h\n> @@ -16,6 +16,7 @@ enum LoggingTarget {\n> \tLoggingTargetSyslog,\n> \tLoggingTargetFile,\n> \tLoggingTargetStream,\n> +\tLoggingTargetCros,\n\nI'm not a big fan of this being exposed in the public API :-(\n\n> };\n> \n> int logSetFile(const char *path, bool color = false);\n> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> index 3a656b8f0..930e2329e 100644\n> --- a/src/libcamera/base/log.cpp\n> +++ b/src/libcamera/base/log.cpp\n> @@ -107,6 +107,7 @@ class LogOutput\n> public:\n> \tLogOutput(const char *path, bool color);\n> \tLogOutput(std::ostream *stream, bool color);\n> +\tLogOutput(bool color);\n> \tLogOutput();\n> \t~LogOutput();\n> \n> @@ -144,6 +145,19 @@ LogOutput::LogOutput(std::ostream *stream, bool color)\n> {\n> }\n> \n> +/**\n> + * \\brief Construct a log output to both std:err and syslog\n\ns/std:err/std::cerr/\n\n> + * \\param[in] color True to output colored messages\n> + *\n> + * Currently this is only needed for CrOS. Therefore, the target is set to\n> + * `LoggingTargetCros`.\n> + */\n> +LogOutput::LogOutput(bool color)\n> +\t: stream_(&std::cerr), target_(LoggingTargetCros), color_(color)\n> +{\n> +\topenlog(\"libcamera\", LOG_PID, 0);\n> +}\n\nBrainstorming mode, I wonder if it wouldn't be cleaner to add support\nfor multiple LogOutput instances. That may be overkill though.\n\n> +\n> /**\n> * \\brief Construct a log output to syslog\n> */\n> @@ -160,6 +174,7 @@ LogOutput::~LogOutput()\n> \t\tdelete stream_;\n> \t\tbreak;\n> \tcase LoggingTargetSyslog:\n> +\tcase LoggingTargetCros:\n> \t\tcloselog();\n> \t\tbreak;\n> \tdefault:\n> @@ -230,17 +245,26 @@ void LogOutput::write(const LogMessage &msg)\n> \t\t\tseverityColor = kColorBrightWhite;\n> \t}\n> \n> +\tbool toStream = false;\n> +\tbool toSyslog = false;\n> +\n> \tswitch (target_) {\n> \tcase LoggingTargetSyslog:\n> -\t\tstr = std::string(log_severity_name(severity)) + \" \"\n> -\t\t + msg.category().name() + \" \" + msg.fileInfo() + \" \";\n> -\t\tif (!msg.prefix().empty())\n> -\t\t\tstr += msg.prefix() + \": \";\n> -\t\tstr += msg.msg();\n> -\t\twriteSyslog(severity, str);\n> +\t\ttoSyslog = true;\n> \t\tbreak;\n> \tcase LoggingTargetStream:\n> \tcase LoggingTargetFile:\n> +\t\ttoStream = true;\n> +\t\tbreak;\n> +\tcase LoggingTargetCros:\n> +\t\ttoSyslog = true;\n> +\t\ttoStream = true;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (toStream) {\n> \t\tstr = \"[\" + utils::time_point_to_string(msg.timestamp()) + \"] [\"\n> \t\t + std::to_string(Thread::currentId()) + \"] \"\n> \t\t + severityColor + log_severity_name(severity) + \" \"\n> @@ -250,9 +274,15 @@ void LogOutput::write(const LogMessage &msg)\n> \t\t\tstr += prefixColor + msg.prefix() + \": \";\n> \t\tstr += resetColor + msg.msg();\n> \t\twriteStream(str);\n> -\t\tbreak;\n> -\tdefault:\n> -\t\tbreak;\n> +\t}\n> +\n> +\tif (toSyslog) {\n> +\t\tstr = std::string(log_severity_name(severity)) + \" \"\n> +\t\t + msg.category().name() + \" \" + msg.fileInfo() + \" \";\n> +\t\tif (!msg.prefix().empty())\n> +\t\t\tstr += msg.prefix() + \": \";\n> +\t\tstr += msg.msg();\n> +\t\twriteSyslog(severity, str);\n> \t}\n> }\n> \n> @@ -270,6 +300,10 @@ void LogOutput::write(const std::string &str)\n> \tcase LoggingTargetFile:\n> \t\twriteStream(str);\n> \t\tbreak;\n> +\tcase LoggingTargetCros:\n> +\t\twriteSyslog(LogDebug, str);\n> +\t\twriteStream(str);\n> +\t\tbreak;\n> \tdefault:\n> \t\tbreak;\n> \t}\n> @@ -549,6 +583,9 @@ int Logger::logSetTarget(enum LoggingTarget target)\n> \tcase LoggingTargetNone:\n> \t\tstd::atomic_store(&output_, std::shared_ptr());\n> \t\tbreak;\n> +\tcase LoggingTargetCros:\n> +\t\tstd::atomic_store(&output_, std::make_shared(true));\n> +\t\tbreak;\n> \tdefault:\n> \t\treturn -EINVAL;\n> \t}\n> @@ -586,6 +623,12 @@ void Logger::logSetLevel(const char *category, const char *level)\n> */\n> Logger::Logger()\n> {\n> +#if defined(OS_CHROMEOS)\n> +\tlogSetTarget(LoggingTargetCros);\n> +\tparseLogLevels();\n> +\treturn;\n> +#endif\n\nI'd like to avoid an #ifdef here. Is there a way to expose this feature\nthrough the libcamera API in a clean way, and select the right target\nusing the API from the HAL code ?\n\n> +\n> \tbool color = !utils::secure_getenv(\"LIBCAMERA_LOG_NO_COLOR\");\n> \tlogSetStream(&std::cerr, color);\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DEAF3C326C\n\tfor ;\n\tTue, 15 Oct 2024 10:22:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8783865387;\n\tTue, 15 Oct 2024 12:22:30 +0200 (CEST)","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 3396D63525\n\tfor ;\n\tTue, 15 Oct 2024 12:22:28 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [185.143.39.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ACEF59CE;\n\tTue, 15 Oct 2024 12:20:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tqWjLEC0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1728987646;\n\tbh=pWLOpuBvAftFfH2mKQbnGxA7IrDMJB/w2fdRLwLEdWs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tqWjLEC0ye4924SIf1zeMSUkPRGcNpjry2zUFGMLJrIExIxvyJ05uAKu03zHeGR7n\n\taRJtGg2AdDVBiISQVpbA17eqJeQlZYbd4QHi77s+vNQdD0MpkoEKokeZ6/LP9CS/74\n\tOomjE0pnibRPO7fimkK77rSla4s3oz/noPfA01Wo=","Date":"Tue, 15 Oct 2024 13:22:25 +0300","From":"Laurent Pinchart ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH 2/2] libcamera: log: add logTargetCros for ChromeOS logs","Message-ID":"<20241015102225.GE5682@pendragon.ideasonboard.com>","References":"<20241014081738.772258-1-chenghaoyang@chromium.org>\n\t<20241014081738.772258-3-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241014081738.772258-3-chenghaoyang@chromium.org>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31747,"web_url":"https://patchwork.libcamera.org/comment/31747/","msgid":"","date":"2024-10-15T11:03:37","subject":"Re: [PATCH 2/2] libcamera: log: add logTargetCros for ChromeOS logs","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Tue, Oct 15, 2024 at 6:22 PM Laurent Pinchart\n wrote:\n>\n> Hi Harvey, Han-Lin,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 14, 2024 at 08:13:55AM +0000, Harvey Yang wrote:\n> > From: Han-Lin Chen \n> >\n> > ChromeOS logging requires that the logs export to both syslog and\n> > std::cerr. Add a logTargetCros for the specific case.\n> >\n> > Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/logging.h | 1 +\n> > src/libcamera/base/log.cpp | 61 +++++++++++++++++++++++++++++++------\n> > 2 files changed, 53 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h\n> > index e1c6341ce..b88acf23b 100644\n> > --- a/include/libcamera/logging.h\n> > +++ b/include/libcamera/logging.h\n> > @@ -16,6 +16,7 @@ enum LoggingTarget {\n> > LoggingTargetSyslog,\n> > LoggingTargetFile,\n> > LoggingTargetStream,\n> > + LoggingTargetCros,\n>\n> I'm not a big fan of this being exposed in the public API :-(\n\nSure, but as you can see, this CL tries to enable multiple\noutputs. The naming can be discussed.\n\n>\n> > };\n> >\n> > int logSetFile(const char *path, bool color = false);\n> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> > index 3a656b8f0..930e2329e 100644\n> > --- a/src/libcamera/base/log.cpp\n> > +++ b/src/libcamera/base/log.cpp\n> > @@ -107,6 +107,7 @@ class LogOutput\n> > public:\n> > LogOutput(const char *path, bool color);\n> > LogOutput(std::ostream *stream, bool color);\n> > + LogOutput(bool color);\n> > LogOutput();\n> > ~LogOutput();\n> >\n> > @@ -144,6 +145,19 @@ LogOutput::LogOutput(std::ostream *stream, bool color)\n> > {\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a log output to both std:err and syslog\n>\n> s/std:err/std::cerr/\n>\n> > + * \\param[in] color True to output colored messages\n> > + *\n> > + * Currently this is only needed for CrOS. Therefore, the target is set to\n> > + * `LoggingTargetCros`.\n> > + */\n> > +LogOutput::LogOutput(bool color)\n> > + : stream_(&std::cerr), target_(LoggingTargetCros), color_(color)\n> > +{\n> > + openlog(\"libcamera\", LOG_PID, 0);\n> > +}\n>\n> Brainstorming mode, I wonder if it wouldn't be cleaner to add support\n> for multiple LogOutput instances. That may be overkill though.\n\nTrue, that's an approach.\n\n>\n> > +\n> > /**\n> > * \\brief Construct a log output to syslog\n> > */\n> > @@ -160,6 +174,7 @@ LogOutput::~LogOutput()\n> > delete stream_;\n> > break;\n> > case LoggingTargetSyslog:\n> > + case LoggingTargetCros:\n> > closelog();\n> > break;\n> > default:\n> > @@ -230,17 +245,26 @@ void LogOutput::write(const LogMessage &msg)\n> > severityColor = kColorBrightWhite;\n> > }\n> >\n> > + bool toStream = false;\n> > + bool toSyslog = false;\n> > +\n> > switch (target_) {\n> > case LoggingTargetSyslog:\n> > - str = std::string(log_severity_name(severity)) + \" \"\n> > - + msg.category().name() + \" \" + msg.fileInfo() + \" \";\n> > - if (!msg.prefix().empty())\n> > - str += msg.prefix() + \": \";\n> > - str += msg.msg();\n> > - writeSyslog(severity, str);\n> > + toSyslog = true;\n> > break;\n> > case LoggingTargetStream:\n> > case LoggingTargetFile:\n> > + toStream = true;\n> > + break;\n> > + case LoggingTargetCros:\n> > + toSyslog = true;\n> > + toStream = true;\n> > + break;\n> > + default:\n> > + break;\n> > + }\n> > +\n> > + if (toStream) {\n> > str = \"[\" + utils::time_point_to_string(msg.timestamp()) + \"] [\"\n> > + std::to_string(Thread::currentId()) + \"] \"\n> > + severityColor + log_severity_name(severity) + \" \"\n> > @@ -250,9 +274,15 @@ void LogOutput::write(const LogMessage &msg)\n> > str += prefixColor + msg.prefix() + \": \";\n> > str += resetColor + msg.msg();\n> > writeStream(str);\n> > - break;\n> > - default:\n> > - break;\n> > + }\n> > +\n> > + if (toSyslog) {\n> > + str = std::string(log_severity_name(severity)) + \" \"\n> > + + msg.category().name() + \" \" + msg.fileInfo() + \" \";\n> > + if (!msg.prefix().empty())\n> > + str += msg.prefix() + \": \";\n> > + str += msg.msg();\n> > + writeSyslog(severity, str);\n> > }\n> > }\n> >\n> > @@ -270,6 +300,10 @@ void LogOutput::write(const std::string &str)\n> > case LoggingTargetFile:\n> > writeStream(str);\n> > break;\n> > + case LoggingTargetCros:\n> > + writeSyslog(LogDebug, str);\n> > + writeStream(str);\n> > + break;\n> > default:\n> > break;\n> > }\n> > @@ -549,6 +583,9 @@ int Logger::logSetTarget(enum LoggingTarget target)\n> > case LoggingTargetNone:\n> > std::atomic_store(&output_, std::shared_ptr());\n> > break;\n> > + case LoggingTargetCros:\n> > + std::atomic_store(&output_, std::make_shared(true));\n> > + break;\n> > default:\n> > return -EINVAL;\n> > }\n> > @@ -586,6 +623,12 @@ void Logger::logSetLevel(const char *category, const char *level)\n> > */\n> > Logger::Logger()\n> > {\n> > +#if defined(OS_CHROMEOS)\n> > + logSetTarget(LoggingTargetCros);\n> > + parseLogLevels();\n> > + return;\n> > +#endif\n>\n> I'd like to avoid an #ifdef here. Is there a way to expose this feature\n> through the libcamera API in a clean way, and select the right target\n> using the API from the HAL code ?\n\nWe're trying not to use environment variables in CrOS, as there will\nbe some confusion and mis-usages.\n\nIt's not a critical patch though. If we couldn't find a good approach,\nwe can skip this. CrOS just keep this in our own branches.\n\nBR,\nHarvey\n\n>\n> > +\n> > bool color = !utils::secure_getenv(\"LIBCAMERA_LOG_NO_COLOR\");\n> > logSetStream(&std::cerr, color);\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 35CB3C326C\n\tfor ;\n\tTue, 15 Oct 2024 11:03:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAD086537E;\n\tTue, 15 Oct 2024 13:03:50 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47F7760537\n\tfor ;\n\tTue, 15 Oct 2024 13:03:49 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id\n\t38308e7fff4ca-2fb4fa17044so18695481fa.3\n\tfor ;\n\tTue, 15 Oct 2024 04:03:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"lYnBr/g0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1728990228; x=1729595028;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Ei8MxDwS49xCsBlPEAJy8k5wPfKcYRAiviQeqTXg65s=;\n\tb=lYnBr/g0xAaInO3BZHRykBXCR74TsQ+L5h2tCK7L8C675qdBXVqHFK+ZSEKzFiHpqZ\n\tpEanNFjEy6jogmRR2zv0YQK7AAyst7D8b75dEEevxLJFBQ8Svk1fXcgY6UmEJmpaPcf2\n\t7JJ8RNTofHGJRz9LfC6KXEeGPIObeLmFn8zKw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1728990228; x=1729595028;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=Ei8MxDwS49xCsBlPEAJy8k5wPfKcYRAiviQeqTXg65s=;\n\tb=jRamD9DyVrQCk/ha0FuCtmfPTGzE87jYztaHRtIhULduV36gNJAVMpZvNNAuKaxvuA\n\t2h2IMisXQ0YUX+3o5HD0s0WcHMa9YErnScbk9uqWdjjzfX9Qtvf8+56xH+IFOJw5hTbi\n\tVBbW+RocqLhC/7qeCNGXezv4uh4PUAPlyWlR+xfnq8H4MAzIqrWw/DFX7DfUXljneRfk\n\tEqhJc9Fm5189XifL4IbobG08yh2pc1iN2nptlZ7DMVfj1xN2nMsN2YIqSY9QBR25QQfs\n\tsgEqn/nPrt1rPXLHL/euyoDNyN5oHgEDqmda2AXLeNZLgekhweRHcu2fu2eFehoTt1ll\n\tMotQ==","X-Gm-Message-State":"AOJu0Yx2nG6Yp1hhxzn5lT39J2OXYnvANz2ka/vNTAIt3RiXdBRDN2hi\n\tY8B4NSXamAaPLz016jfWrSeu9QmRtys3M7+LvR06PSawzqaos03NDo6YywYUE283yVvnh8JwXYo\n\t2imGeBCkGplEV9dlympx2v7sRMouBI2TmbIWZ","X-Google-Smtp-Source":"AGHT+IE3cJdNFN9lTn5Rdf0ukuusmkzId79TKQgeQb2qhPA5FioBqCJ35vNOEWkbnAyaszhD4wY5tjBN7D8OB+dop4o=","X-Received":"by 2002:a2e:b8c6:0:b0:2fb:565a:d918 with SMTP id\n\t38308e7fff4ca-2fb565adc3bmr31301191fa.12.1728990228144;\n\tTue, 15 Oct 2024 04:03:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20241014081738.772258-1-chenghaoyang@chromium.org>\n\t<20241014081738.772258-3-chenghaoyang@chromium.org>\n\t<20241015102225.GE5682@pendragon.ideasonboard.com>","In-Reply-To":"<20241015102225.GE5682@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Tue, 15 Oct 2024 19:03:37 +0800","Message-ID":"","Subject":"Re: [PATCH 2/2] libcamera: log: add logTargetCros for ChromeOS logs","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]