[PATCH 2/2] libcamera: log: add logTargetCros for ChromeOS logs

[Cheng-Hao Yang]

Hi Laurent,

On Tue, Oct 15, 2024 at 6:22 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Harvey, Han-Lin,
>
> Thank you for the patch.
>
> On Mon, Oct 14, 2024 at 08:13:55AM +0000, Harvey Yang wrote:
> > From: Han-Lin Chen <hanlinchen at chromium.org>
> >
> > ChromeOS logging requires that the logs export to both syslog and
> > std::cerr. Add a logTargetCros for the specific case.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  include/libcamera/logging.h |  1 +
> >  src/libcamera/base/log.cpp  | 61 +++++++++++++++++++++++++++++++------
> >  2 files changed, 53 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/logging.h b/include/libcamera/logging.h
> > index e1c6341ce..b88acf23b 100644
> > --- a/include/libcamera/logging.h
> > +++ b/include/libcamera/logging.h
> > @@ -16,6 +16,7 @@ enum LoggingTarget {
> >       LoggingTargetSyslog,
> >       LoggingTargetFile,
> >       LoggingTargetStream,
> > +     LoggingTargetCros,
>
> I'm not a big fan of this being exposed in the public API :-(

Sure, but as you can see, this CL tries to enable multiple
outputs. The naming can be discussed.

>
> >  };
> >
> >  int logSetFile(const char *path, bool color = false);
> > diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> > index 3a656b8f0..930e2329e 100644
> > --- a/src/libcamera/base/log.cpp
> > +++ b/src/libcamera/base/log.cpp
> > @@ -107,6 +107,7 @@ class LogOutput
> >  public:
> >       LogOutput(const char *path, bool color);
> >       LogOutput(std::ostream *stream, bool color);
> > +     LogOutput(bool color);
> >       LogOutput();
> >       ~LogOutput();
> >
> > @@ -144,6 +145,19 @@ LogOutput::LogOutput(std::ostream *stream, bool color)
> >  {
> >  }
> >
> > +/**
> > + * \brief Construct a log output to both std:err and syslog
>
> s/std:err/std::cerr/
>
> > + * \param[in] color True to output colored messages
> > + *
> > + * Currently this is only needed for CrOS. Therefore, the target is set to
> > + * `LoggingTargetCros`.
> > + */
> > +LogOutput::LogOutput(bool color)
> > +     : stream_(&std::cerr), target_(LoggingTargetCros), color_(color)
> > +{
> > +     openlog("libcamera", LOG_PID, 0);
> > +}
>
> Brainstorming mode, I wonder if it wouldn't be cleaner to add support
> for multiple LogOutput instances. That may be overkill though.

True, that's an approach.

>
> > +
> >  /**
> >   * \brief Construct a log output to syslog
> >   */
> > @@ -160,6 +174,7 @@ LogOutput::~LogOutput()
> >               delete stream_;
> >               break;
> >       case LoggingTargetSyslog:
> > +     case LoggingTargetCros:
> >               closelog();
> >               break;
> >       default:
> > @@ -230,17 +245,26 @@ void LogOutput::write(const LogMessage &msg)
> >                       severityColor = kColorBrightWhite;
> >       }
> >
> > +     bool toStream = false;
> > +     bool toSyslog = false;
> > +
> >       switch (target_) {
> >       case LoggingTargetSyslog:
> > -             str = std::string(log_severity_name(severity)) + " "
> > -                 + msg.category().name() + " " + msg.fileInfo() + " ";
> > -             if (!msg.prefix().empty())
> > -                     str += msg.prefix() + ": ";
> > -             str += msg.msg();
> > -             writeSyslog(severity, str);
> > +             toSyslog = true;
> >               break;
> >       case LoggingTargetStream:
> >       case LoggingTargetFile:
> > +             toStream = true;
> > +             break;
> > +     case LoggingTargetCros:
> > +             toSyslog = true;
> > +             toStream = true;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     if (toStream) {
> >               str = "[" + utils::time_point_to_string(msg.timestamp()) + "] ["
> >                   + std::to_string(Thread::currentId()) + "] "
> >                   + severityColor + log_severity_name(severity) + " "
> > @@ -250,9 +274,15 @@ void LogOutput::write(const LogMessage &msg)
> >                       str += prefixColor + msg.prefix() + ": ";
> >               str += resetColor + msg.msg();
> >               writeStream(str);
> > -             break;
> > -     default:
> > -             break;
> > +     }
> > +
> > +     if (toSyslog) {
> > +             str = std::string(log_severity_name(severity)) + " "
> > +                 + msg.category().name() + " " + msg.fileInfo() + " ";
> > +             if (!msg.prefix().empty())
> > +                     str += msg.prefix() + ": ";
> > +             str += msg.msg();
> > +             writeSyslog(severity, str);
> >       }
> >  }
> >
> > @@ -270,6 +300,10 @@ void LogOutput::write(const std::string &str)
> >       case LoggingTargetFile:
> >               writeStream(str);
> >               break;
> > +     case LoggingTargetCros:
> > +             writeSyslog(LogDebug, str);
> > +             writeStream(str);
> > +             break;
> >       default:
> >               break;
> >       }
> > @@ -549,6 +583,9 @@ int Logger::logSetTarget(enum LoggingTarget target)
> >       case LoggingTargetNone:
> >               std::atomic_store(&output_, std::shared_ptr<LogOutput>());
> >               break;
> > +     case LoggingTargetCros:
> > +             std::atomic_store(&output_, std::make_shared<LogOutput>(true));
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -586,6 +623,12 @@ void Logger::logSetLevel(const char *category, const char *level)
> >   */
> >  Logger::Logger()
> >  {
> > +#if defined(OS_CHROMEOS)
> > +     logSetTarget(LoggingTargetCros);
> > +     parseLogLevels();
> > +     return;
> > +#endif
>
> I'd like to avoid an #ifdef here. Is there a way to expose this feature
> through the libcamera API in a clean way, and select the right target
> using the API from the HAL code ?

We're trying not to use environment variables in CrOS, as there will
be some confusion and mis-usages.

It's not a critical patch though. If we couldn't find a good approach,
we can skip this. CrOS just keep this in our own branches.

BR,
Harvey

>
> > +
> >       bool color = !utils::secure_getenv("LIBCAMERA_LOG_NO_COLOR");
> >       logSetStream(&std::cerr, color);
> >
>
> --
> Regards,
>
> Laurent Pinchart