[libcamera-devel] libcamera: keep using syslog logging target also for isolated IPA modules
diff mbox series

Message ID 20230220074352.1221623-1-matthias.fend@emfend.at
State Accepted
Commit dbe96a2a6b130f7a3ffa1ad037b96cbbd7514956
Headers show
Series
  • [libcamera-devel] libcamera: keep using syslog logging target also for isolated IPA modules
Related show

Commit Message

Matthias Fend Feb. 20, 2023, 7:43 a.m. UTC 
Currently it is not possible to display debug output from an isolated IPA
module. The standard  descriptors are all closed and any specified log
file is explicitly deactivated for the IPA module. Since libcamera and the
isolated IPA modul are separate processes, they cannot write to the same
file. However, if syslog is used, then this would be possible.

If syslog is specified as a log file, then this is left as it is for the
isolated IPA module.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 src/libcamera/process.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Feb. 20, 2023, 8:50 a.m. UTC | #1 
Hello Matthias

On Mon, Feb 20, 2023 at 08:43:52AM +0100, Matthias Fend via libcamera-devel wrote:
> Currently it is not possible to display debug output from an isolated IPA
> module. The standard  descriptors are all closed and any specified log
> file is explicitly deactivated for the IPA module. Since libcamera and the
> isolated IPA modul are separate processes, they cannot write to the same
> file. However, if syslog is used, then this would be possible.
>
> If syslog is specified as a log file, then this is left as it is for the
> isolated IPA module.
>

I'm certainly missing something, but doesn't logging to syslogd
happens through the 'syslog()'[1] function call ?

[1] https://linux.die.net/man/3/syslog

> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  src/libcamera/process.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 0e6b4e1d..86a382fb 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
>
>  		closeAllFdsExcept(fds);
>
> -		unsetenv("LIBCAMERA_LOG_FILE");
> +		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> +		if (file && strcmp(file, "syslog"))
> +			unsetenv("LIBCAMERA_LOG_FILE");
>
>  		const char **argv = new const char *[args.size() + 2];
>  		unsigned int len = args.size();
> --
> 2.25.1
>
Matthias Fend Feb. 20, 2023, 9:13 a.m. UTC | #2 
Hi Jacopo,

Am 20.02.2023 um 09:50 schrieb Jacopo Mondi:
> Hello Matthias
> 
> On Mon, Feb 20, 2023 at 08:43:52AM +0100, Matthias Fend via libcamera-devel wrote:
>> Currently it is not possible to display debug output from an isolated IPA
>> module. The standard  descriptors are all closed and any specified log
>> file is explicitly deactivated for the IPA module. Since libcamera and the
>> isolated IPA modul are separate processes, they cannot write to the same
>> file. However, if syslog is used, then this would be possible.
>>
>> If syslog is specified as a log file, then this is left as it is for the
>> isolated IPA module.
>>
> 
> I'm certainly missing something, but doesn't logging to syslogd
> happens through the 'syslog()'[1] function call ?

If LIBCAMERA_LOG_FILE is set to 'syslog', the logging target is set to 
'LoggingTargetSyslog', which instructs the libcamera logger to use 
syslog() to output log messages. This is already implemented.

However, this environment variable is explicitly deleted for an isolated 
IPA module. This change only prevents this behavior if syslog is to be used.

I don't see any other way to see the debug output of such a module - 
apart from adapting the libcamera source code of course.

~Matthias

> 
> [1] https://linux.die.net/man/3/syslog
> 
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   src/libcamera/process.cpp | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
>> index 0e6b4e1d..86a382fb 100644
>> --- a/src/libcamera/process.cpp
>> +++ b/src/libcamera/process.cpp
>> @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
>>
>>   		closeAllFdsExcept(fds);
>>
>> -		unsetenv("LIBCAMERA_LOG_FILE");
>> +		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>> +		if (file && strcmp(file, "syslog"))
>> +			unsetenv("LIBCAMERA_LOG_FILE");
>>
>>   		const char **argv = new const char *[args.size() + 2];
>>   		unsigned int len = args.size();
>> --
>> 2.25.1
>>
Jacopo Mondi Feb. 20, 2023, 10:01 a.m. UTC | #3 
Hello Matthias

On Mon, Feb 20, 2023 at 10:13:06AM +0100, Matthias Fend wrote:
> Hi Jacopo,
>
> Am 20.02.2023 um 09:50 schrieb Jacopo Mondi:
> > Hello Matthias
> >
> > On Mon, Feb 20, 2023 at 08:43:52AM +0100, Matthias Fend via libcamera-devel wrote:
> > > Currently it is not possible to display debug output from an isolated IPA
> > > module. The standard  descriptors are all closed and any specified log
> > > file is explicitly deactivated for the IPA module. Since libcamera and the
> > > isolated IPA modul are separate processes, they cannot write to the same
> > > file. However, if syslog is used, then this would be possible.
> > >
> > > If syslog is specified as a log file, then this is left as it is for the
> > > isolated IPA module.
> > >
> >
> > I'm certainly missing something, but doesn't logging to syslogd
> > happens through the 'syslog()'[1] function call ?
>
> If LIBCAMERA_LOG_FILE is set to 'syslog', the logging target is set to
> 'LoggingTargetSyslog', which instructs the libcamera logger to use syslog()
> to output log messages. This is already implemented.

Ups, I completely missed that

>
> However, this environment variable is explicitly deleted for an isolated IPA
> module. This change only prevents this behavior if syslog is to be used.
>
> I don't see any other way to see the debug output of such a module - apart
> from adapting the libcamera source code of course.
>

Ok then, this now makes sense to me.

I don't see issues in allowing isolated processes interface to syslog,
but let's wait for other's opinions.

> ~Matthias
>
> >
> > [1] https://linux.die.net/man/3/syslog
> >
> > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > ---
> > >   src/libcamera/process.cpp | 4 +++-
> > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> > > index 0e6b4e1d..86a382fb 100644
> > > --- a/src/libcamera/process.cpp
> > > +++ b/src/libcamera/process.cpp
> > > @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
> > >
> > >   		closeAllFdsExcept(fds);
> > >
> > > -		unsetenv("LIBCAMERA_LOG_FILE");
> > > +		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> > > +		if (file && strcmp(file, "syslog"))
> > > +			unsetenv("LIBCAMERA_LOG_FILE");

Indentation seems off though

> > >
> > >   		const char **argv = new const char *[args.size() + 2];
> > >   		unsigned int len = args.size();
> > > --
> > > 2.25.1
> > >
Matthias Fend Feb. 24, 2023, 11:01 a.m. UTC | #4 
Hi Jacopo,

Am 20.02.2023 um 11:01 schrieb Jacopo Mondi:
> Hello Matthias
> 
> On Mon, Feb 20, 2023 at 10:13:06AM +0100, Matthias Fend wrote:
>> Hi Jacopo,
>>
>> Am 20.02.2023 um 09:50 schrieb Jacopo Mondi:
>>> Hello Matthias
>>>
>>> On Mon, Feb 20, 2023 at 08:43:52AM +0100, Matthias Fend via libcamera-devel wrote:
>>>> Currently it is not possible to display debug output from an isolated IPA
>>>> module. The standard  descriptors are all closed and any specified log
>>>> file is explicitly deactivated for the IPA module. Since libcamera and the
>>>> isolated IPA modul are separate processes, they cannot write to the same
>>>> file. However, if syslog is used, then this would be possible.
>>>>
>>>> If syslog is specified as a log file, then this is left as it is for the
>>>> isolated IPA module.
>>>>
>>>
>>> I'm certainly missing something, but doesn't logging to syslogd
>>> happens through the 'syslog()'[1] function call ?
>>
>> If LIBCAMERA_LOG_FILE is set to 'syslog', the logging target is set to
>> 'LoggingTargetSyslog', which instructs the libcamera logger to use syslog()
>> to output log messages. This is already implemented.
> 
> Ups, I completely missed that
> 
>>
>> However, this environment variable is explicitly deleted for an isolated IPA
>> module. This change only prevents this behavior if syslog is to be used.
>>
>> I don't see any other way to see the debug output of such a module - apart
>> from adapting the libcamera source code of course.
>>
> 
> Ok then, this now makes sense to me.
> 
> I don't see issues in allowing isolated processes interface to syslog,
> but let's wait for other's opinions.
> 
>> ~Matthias
>>
>>>
>>> [1] https://linux.die.net/man/3/syslog
>>>
>>>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>>>> ---
>>>>    src/libcamera/process.cpp | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
>>>> index 0e6b4e1d..86a382fb 100644
>>>> --- a/src/libcamera/process.cpp
>>>> +++ b/src/libcamera/process.cpp
>>>> @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
>>>>
>>>>    		closeAllFdsExcept(fds);
>>>>
>>>> -		unsetenv("LIBCAMERA_LOG_FILE");
>>>> +		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
>>>> +		if (file && strcmp(file, "syslog"))
>>>> +			unsetenv("LIBCAMERA_LOG_FILE");
> 
> Indentation seems off though

Unfortunately, I do not see the problem. Can you please tell me more 
precisely what does not fit?

Thanks
  ~Matthias

> 
>>>>
>>>>    		const char **argv = new const char *[args.size() + 2];
>>>>    		unsigned int len = args.size();
>>>> --
>>>> 2.25.1
>>>>
Jacopo Mondi Feb. 24, 2023, 12:46 p.m. UTC | #5 
Hi Matthias

On Fri, Feb 24, 2023 at 12:01:33PM +0100, Matthias Fend via libcamera-devel wrote:
> Hi Jacopo,
>
> Am 20.02.2023 um 11:01 schrieb Jacopo Mondi:
> > Hello Matthias
> >
> > On Mon, Feb 20, 2023 at 10:13:06AM +0100, Matthias Fend wrote:
> > > Hi Jacopo,
> > >
> > > Am 20.02.2023 um 09:50 schrieb Jacopo Mondi:
> > > > Hello Matthias
> > > >
> > > > On Mon, Feb 20, 2023 at 08:43:52AM +0100, Matthias Fend via libcamera-devel wrote:
> > > > > Currently it is not possible to display debug output from an isolated IPA
> > > > > module. The standard  descriptors are all closed and any specified log
> > > > > file is explicitly deactivated for the IPA module. Since libcamera and the
> > > > > isolated IPA modul are separate processes, they cannot write to the same
> > > > > file. However, if syslog is used, then this would be possible.
> > > > >
> > > > > If syslog is specified as a log file, then this is left as it is for the
> > > > > isolated IPA module.
> > > > >
> > > >
> > > > I'm certainly missing something, but doesn't logging to syslogd
> > > > happens through the 'syslog()'[1] function call ?
> > >
> > > If LIBCAMERA_LOG_FILE is set to 'syslog', the logging target is set to
> > > 'LoggingTargetSyslog', which instructs the libcamera logger to use syslog()
> > > to output log messages. This is already implemented.
> >
> > Ups, I completely missed that
> >
> > >
> > > However, this environment variable is explicitly deleted for an isolated IPA
> > > module. This change only prevents this behavior if syslog is to be used.
> > >
> > > I don't see any other way to see the debug output of such a module - apart
> > > from adapting the libcamera source code of course.
> > >
> >
> > Ok then, this now makes sense to me.
> >
> > I don't see issues in allowing isolated processes interface to syslog,
> > but let's wait for other's opinions.
> >
> > > ~Matthias
> > >
> > > >
> > > > [1] https://linux.die.net/man/3/syslog
> > > >
> > > > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > > > ---
> > > > >    src/libcamera/process.cpp | 4 +++-
> > > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> > > > > index 0e6b4e1d..86a382fb 100644
> > > > > --- a/src/libcamera/process.cpp
> > > > > +++ b/src/libcamera/process.cpp
> > > > > @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
> > > > >
> > > > >    		closeAllFdsExcept(fds);
> > > > >
> > > > > -		unsetenv("LIBCAMERA_LOG_FILE");
> > > > > +		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> > > > > +		if (file && strcmp(file, "syslog"))
> > > > > +			unsetenv("LIBCAMERA_LOG_FILE");
> >
> > Indentation seems off though
>
> Unfortunately, I do not see the problem. Can you please tell me more
> precisely what does not fit?
>

I'm sorry don't know what happened but in my client it was shown with
a single indendentation tab :/

Sorry for the noise

> Thanks
>  ~Matthias
>
> >
> > > > >
> > > > >    		const char **argv = new const char *[args.size() + 2];
> > > > >    		unsigned int len = args.size();
> > > > > --
> > > > > 2.25.1
> > > > >
Kieran Bingham March 1, 2023, 12:29 a.m. UTC | #6 
Quoting Matthias Fend via libcamera-devel (2023-02-20 07:43:52)
> Currently it is not possible to display debug output from an isolated IPA
> module. The standard  descriptors are all closed and any specified log
> file is explicitly deactivated for the IPA module. Since libcamera and the
> isolated IPA modul are separate processes, they cannot write to the same
> file. However, if syslog is used, then this would be possible.
> 
> If syslog is specified as a log file, then this is left as it is for the
> isolated IPA module.
> 

This seems reasonable to me.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  src/libcamera/process.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 0e6b4e1d..86a382fb 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
>  
>                 closeAllFdsExcept(fds);
>  
> -               unsetenv("LIBCAMERA_LOG_FILE");
> +               const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> +               if (file && strcmp(file, "syslog"))
> +                       unsetenv("LIBCAMERA_LOG_FILE");
>  
>                 const char **argv = new const char *[args.size() + 2];
>                 unsigned int len = args.size();
> -- 
> 2.25.1
>
Jacopo Mondi March 1, 2023, 3:36 p.m. UTC | #7 
Hello Matthias

On Mon, Feb 20, 2023 at 08:43:52AM +0100, Matthias Fend via libcamera-devel wrote:
> Currently it is not possible to display debug output from an isolated IPA
> module. The standard  descriptors are all closed and any specified log
> file is explicitly deactivated for the IPA module. Since libcamera and the
> isolated IPA modul are separate processes, they cannot write to the same
> file. However, if syslog is used, then this would be possible.
>
> If syslog is specified as a log file, then this is left as it is for the
> isolated IPA module.
>
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>

Thanks for all the clarifications
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Tested-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

We'll merge this one soon

> ---
>  src/libcamera/process.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
> index 0e6b4e1d..86a382fb 100644
> --- a/src/libcamera/process.cpp
> +++ b/src/libcamera/process.cpp
> @@ -263,7 +263,9 @@ int Process::start(const std::string &path,
>
>  		closeAllFdsExcept(fds);
>
> -		unsetenv("LIBCAMERA_LOG_FILE");
> +		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
> +		if (file && strcmp(file, "syslog"))
> +			unsetenv("LIBCAMERA_LOG_FILE");
>
>  		const char **argv = new const char *[args.size() + 2];
>  		unsigned int len = args.size();
> --
> 2.25.1
>

Patch
diff mbox series 

diff --git a/src/libcamera/process.cpp b/src/libcamera/process.cpp
index 0e6b4e1d..86a382fb 100644
--- a/src/libcamera/process.cpp
+++ b/src/libcamera/process.cpp
@@ -263,7 +263,9 @@  int Process::start(const std::string &path,
 
 		closeAllFdsExcept(fds);
 
-		unsetenv("LIBCAMERA_LOG_FILE");
+		const char *file = utils::secure_getenv("LIBCAMERA_LOG_FILE");
+		if (file && strcmp(file, "syslog"))
+			unsetenv("LIBCAMERA_LOG_FILE");
 
 		const char **argv = new const char *[args.size() + 2];
 		unsigned int len = args.size();